Skip to content

Conversation

@jdmpapin
Copy link
Contributor

The verbose log can now be locked by declaring a local variable of type TR_VerboseLog::CriticalSection, and the lock will be automatically released at the end of the variable's scope. Locking this way is advantageous because it's not possible to forget to release the lock, and because the lock will always be released even if an exception is thrown.

In general, releasing a lock while unexpectedly unwinding for an exception could leave a program in a bad state. If the lock protects an invariant, it's possible to take the lock, temporarily violate the invariant, and then unwind before restoring it. However, in the case of the verbose log lock specifically, the only effect of unlocking on unwind will be to prematurely truncate the output being generated at the time. The output is just diagnostic, so it's best to simply release the lock.

There is an optional boolean parameter to specify whether or not to actually acquire the lock. This is useful for cases where we are incrementally generating output that needs to stay together, but we're doing so conditionally in the midst of other logic, e.g.

if (verbose)
    TR_VerboseLog::write(...);

...

if (verbose)
    TR_VerboseLog::write(...);

...

if (verbose)
    TR_VerboseLog::writeLine(...);

The boolean parameter allows for a sequence like this to use CriticalSection while still only locking when necessary:

TR_VerboseLog::CriticalSection vlogLock(verbose);

Without it, the use of CriticalSection would force the code either to lock each write individually, which is too fine a granularity and allows unwanted interleaving, or to take the lock even when not outputting to the verbose log.


Pairs of calls to vlogAcquire() and vlogRelease() are changed to use CriticalSection instead.

The verbose log can now be locked by declaring a local variable of type
TR_VerboseLog::CriticalSection, and the lock will be automatically
released at the end of the variable's scope. Locking this way is
advantageous because it's not possible to forget to release the lock,
and because the lock will always be released even if an exception is
thrown.

In general, releasing a lock while unexpectedly unwinding for an
exception could leave a program in a bad state. If the lock protects an
invariant, it's possible to take the lock, temporarily violate the
invariant, and then unwind before restoring it. However, in the case of
the verbose log lock specifically, the only effect of unlocking on
unwind will be to prematurely truncate the output being generated at the
time. The output is just diagnostic, so it's best to simply release the
lock.

There is an optional boolean parameter to specify whether or not to
actually acquire the lock. This is useful for cases where we are
incrementally generating output that needs to stay together, but we're
doing so conditionally in the midst of other logic, e.g.

    if (verbose)
        TR_VerboseLog::write(...);

    ...

    if (verbose)
        TR_VerboseLog::write(...);

    ...

    if (verbose)
        TR_VerboseLog::writeLine(...);

The boolean parameter allows for a sequence like this to use
CriticalSection while still only locking when necessary:

    TR_VerboseLog::CriticalSection vlogLock(verbose);

Without it, the use of CriticalSection would force the code either to
lock each write individually, which is too fine a granularity and allows
unwanted interleaving, or to take the lock even when not outputting to
the verbose log.
TR_VerboseLog::CriticalSection should be preferred whenever it is
straightforward to use.
@dsouzai
Copy link
Contributor

dsouzai commented Nov 18, 2021

Jenkins build all

@dsouzai dsouzai self-assigned this Nov 18, 2021
@jdmpapin
Copy link
Contributor Author

"All nodes of label ‘compile:aix’ are offline"

@dsouzai dsouzai merged commit 8321437 into eclipse-omr:master Nov 19, 2021
@jdmpapin jdmpapin deleted the vlog-critical-section branch December 10, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants