On Thu, 12 Nov 2020 08:11:10 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> As the stack trace in the bug shows, we cannot load classes, since we may
>> take a monitor.
>> Resulting in an unexpected result to GetCurrentContendedMonitor().
>> Trying to use some decent primitive, e.g. Wicket/Semaphore/.., without being
>> implementation dependent means we must warm up every possible scenario,
>> since it may use a new class.
>> Instead I here just use sleep + volatile for the barriers.
>>
>> I cannot reproduce with these changes.
>>
>> Chewing through T6 as most issues have been seen there - passed.
>
> Robbin Ehn has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fixed comment
Hi Robbin,
Please see the bug report for more discussion.
Bottom line: I now agree this is the right kind of fix for this situation. I
could nit pick on the variable naming but lets just get this done.
Thanks for your patience on this. It is important to fully understand how these
situations can arise.
David
test/hotspot/jtreg/vmTestbase/nsk/jvmti/GetCurrentContendedMonitor/contmon001.java
line 67:
> 65: public static int run(String argv[], PrintStream ref) {
> 66: out = ref;
> 67: doSleep(); // If we need to load any classes to execute
> doSleep(), do it now.
Well intentioned but not really useful. The classes used on the normal
execution path are already loaded during VM initialization. The exceptional
paths can still lead to class loading/linking/synchronization, so this
preemptive call doesn't help that case anyway.
-------------
Marked as reviewed by dholmes (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1177
