On Fri, 5 May 2023 18:38:28 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Several Handlers class use monitors to synchronize when formatting / >> publishing LogRecords. >> When logging within a VirtualThread, holding this monitor can cause the >> carrier thread to be pinned. >> Handlers could use jdk.internal.misc.InternalLock, in a similar way to some >> java.io classes (such as PrintStream) to avoid pinning the carrier thread. > > Daniel Fuchs has updated the pull request incrementally with one additional > commit since the last revision: > > Use locks consistently Marked as reviewed by jpai (Reviewer). Hello Daniel, from what I see in these changes, the proposed idea is to use `java.util.concurrent.locks.ReentrantLock` by default wherever `synchronized` was used in these `Handler`s. David already noted that these current synchronizations appear to be to serialize the calls. With the use of the proposed internal `ReentrantLock`s the serialization of the calls would still be maintained when dealing with non-subclassed `Handler`s. Use of this internal `ReentrantLock` lock is only enabled if the `Handler` class is loaded by the platform classloader. For any other (sub-classed) `Handler`, these existing methods will still continue to use `synchronized` and thus shouldn't have any impact on the call serialization semantics for such classes. This change looks OK to me. ------------- PR Review: https://git.openjdk.org/jdk/pull/13832#pullrequestreview-1421875299 PR Comment: https://git.openjdk.org/jdk/pull/13832#issuecomment-1543436623
