RFR 8242263: Diagnose synchronization on primitive wrappers

Tue Aug 4 05:11:27 UTC 2020

Hi David,

Thanks for looking at this.

On 8/3/20 3:12 AM, David Holmes wrote:
> Hi Patricio,
>
> I've cc'd JFR dev folk as well.
>
> Thanks for working on this Valhalla-preview support work. I'm not 
> quite clear if this part of the work is intended to be an isolated and 
> independent change relative to the broader scope of work outlined in 
> the draft JEP: JDK-8249100
>
> https://bugs.openjdk.java.net/browse/JDK-8249100
>
> so I'm seeking clarification on that from the Valhalla folk.
>
> Meanwhile ...
>
> On 29/07/2020 5:16 am, Patricio Chilano wrote:
>> Hi all,
>>
>> Please review the following change that adds diagnostic capabilities 
>> when synchronizing on primitive wrapper classes.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8242263
>> Webrev: http://cr.openjdk.java.net/~pchilanomate/8242263/v1/webrev/
>>
>> The new flag allows to identify synchronization on these classes and 
>> to take one of the following actions: exit the VM with fatal error, 
>> log a warning message, or issue a JFR event. 
>
> I'm not clear how conditionally enabled events should be exposed in 
> JFR so will need some input from JFR folk on that part.
Sounds good.

>> The implementation uses a simple approach where a check is added at 
>> the beginning of the monitorenter generated code when the flag is 
>> enabled to check whether the object is of a primitive wrapper class. 
>> If it is, we jump to the slow path, 
>
> I like this simple approach!
>
>> otherwise we just continue as always. The extra instructions will be: 
>> load the klass of the object, load the _access_flags field for that 
>> klass, AND with a constant, and branch based on the result. The code 
>> will only be generated whenever the new opt-in diagnostic flag is 
>> enabled so performance won't be affected when off.
>
> In some cases you've factored out code that was only executed 
> with/without biased-locking so that it is always executed and so 
> available for your new code e.g
>
> src/hotspot/cpu/arm/c1_MacroAssembler_arm.cpp
>
> -   if (UseBiasedLocking) {
> -     // load object
>       str(obj, Address(disp_hdr, obj_offset));
> -     null_check_offset = biased_locking_enter(obj, hdr/*scratched*/, 
> tmp1, false, tmp2, done, slow_case);
> -   }
>
> but the same code on Aarch64 already does the same thing, so 
> performance wise this shouldn't be a concern. 
But I removed a later "str(obj, Address(disp_hdr, obj_offset));" in 
c1_MacroAssembler_arm.cpp, so it should be the same. I basically changed 
the code to match the pattern we have for other platforms where the 
first thing we do is store the oop in the BasicObjectLock object. In 
fact in arm32 now we are executing that store twice when biased locking 
is enabled.

> On the other hand the change from
>
> null_check_offset = biased_locking_enter(...)
>
> to:
>
> null_check_offset = offset();
> biased_locking_enter(...)
>
> does seem to penalize the biased-locking code. Can you not just do:
>
>    if (DiagnoseSyncOnPrimitiveWrappers) {
>  +   null_check_offset = offset();
>      load_klass(hdr, obj);
>      ldr(hdr, Address(hdr, Klass::access_flags_offset()));
>      tst(hdr, JVM_ACC_IS_BOX_CLASS);
>      br(Assembler::NE, slow_case);
>    }
>
> so that only the new code is impacted?
The call to offset() should not generate extra code, it is only executed 
by the compiler thread. I think this is to know where to put the code 
that tests for a NULL oop, and it has to be done right before the actual 
dereference of the oop.  I see what you mean though that we are making a 
call to offset() again in biased_locking_enter() and discarding the 
value. Maybe I can remove those calls instead? I just don't see the 
point in having "null_check_offset = offset()" inside conditionals since 
regardless of the flags we pass we always dereference the oop right at 
the beginning (after storing the object in the BasicObjectLock).

> ---
>
> General comment - you use this a lot:
>
> if (DiagnoseSyncOnPrimitiveWrappers)
>
> but DiagnoseSyncOnPrimitiveWrappers is not a boolean and hotspot-style 
> says to avoid implicit booleans, so those checks need to be expanded to:
>
> if (DiagnoseSyncOnPrimitiveWrappers != 0)
Fixed.

> ---
>
> src/hotspot/share/runtime/synchronizer.cpp
>
> To avoid the need to re-manifest the current thread you can do:
>
> void ObjectSynchronizer::handle_sync_on_primitive_wrapper(Handle obj, 
> Thread* current) {
>    assert(current->is_Java_thread(), "must be for java object 
> synchronization");
>    JavaThread* self = (JavaThread) current;
Done.

>  805 assert(!do_lock || DiagnoseSyncOnPrimitiveWrappers != 
> ObjectSynchronizer::FATAL_EXIT || !obj->klass()->is_box(), 
> "ObjectLocker synchronizing on primitive wrapper");
>
> I don't quite follow that assertion - why is the middle part not just 
> "DiagnoseSyncOnPrimitiveWrappers != 0" ? Or why not just assert it is 
> not a box so that we locate all places where we do sync on boxes?
Ah, that was a leftover from when "1" meant throw IMSE exception. It 
didn't made sense to me to throw an IMSE exception there since that was 
an internal synchronization attempt, so I was asserting instead. I 
removed that line.

> ---
>
> test/hotspot/jtreg/runtime/Monitor/SyncOnPrimitiveWrapperTest.java
>
> Please add "@bug 8242263".
Done.

>  32  * @run main/othervm SyncOnPrimitiveWrapperTest
>
> As the test only launches other VMs this can just be:
>  32  * @run driver SyncOnPrimitiveWrapperTest
Done.

> It would be nice to factor out the common options rather than 
> repeating so many of the string literals.
Added generateTests() to deal with that.

> I don't think you want to use ProcessTools.createTestJvm as that will 
> apply flags from the test run that will potentially conflict with the 
> explicit flag combinations you are testing. Just use 
> ProcessTools.createJavaProcessBuilder.
Fixed.

> Both the FatalTest and the LogTest should be testing wrappers of each 
> type.
Done. LogTest was already testing all the types so I only had to add 
them for FatalTest. Since testing each type for the FatalTest requires 
spawning a new VM each time, I implemented that in generateTests() as 
additional tests with one more argument (an index to the testObjects list).

>  99 Integer integer = new Integer(10);
>
> Use Integer.valueOf(10) (or allow implicit boxing) as direct 
> construction of wrapper types is something that will itself be subject 
> to warnings in the future.
Ok, as part of the above changes I'm now testing the objects in 
testObjects list which use valueOf().

> 101 throw new RuntimeException("synchronization on primitve wrapper 
> did not failed");
>
> typos: primitve -> primitive
>        failed -> fail
Fixed.

> I don't understand why the Log test is so complex. I just expected to 
> see a simple "synchronized(o){}" for an 'o' of each primitive type, 
> and a check that the log contained the expected message.
I added the "for" loop to test C2 properly since I found the special 
flags alone are not enough. I also added additional threads so that 
contention paths are also exercised. With the nested synchronized 
statements the idea was to test more than the simple case and test 
recursive locking too. Maybe I can remove the nested synchronized 
statements?

(I changed "sharedCounter += 2;" to just an increment since it otherwise 
might confuse people reading it that it has some special meaning. I 
think it did in some prior version).

> ---
>
>  test/jdk/jdk/jfr/event/runtime/TestSyncOnPrimitiveWrapperEvent.java
>
> The licence header should not be the version with the classpath 
> exception.
Fixed.

> Please add "@bug 8242263".
Done.

> The test should test each of the wrapper types, not just Integer
Done.

I also realized I should have added -XX:StartFlightRecording to the test 
otherwise right now it asserts because of the check I added in 
arguments.cpp. In my tests it never failed because I had a small error 
where I had an extra closing bracket inside the #if INCLUDE_JFR macro in 
arguments.cpp that I fixed before sending v1. In any case, now that I 
look at it I think it doesn't make sense to enforce 
-XX:StartFlightRecording since it can work without it. So I just removed 
that extra check in arguments.cpp. Let's see also what the JFR folk say.

>> In addition to the purpose described in the description of the bug, 
>> this flag will also be useful when trying to diagnose possible 
>> synchronization issues if these classes ever become inline types as 
>> part of the Valhalla project.
>>
>> I added test SyncOnPrimitiveWrapperTest.java that tests for the exit 
>> and logging cases. I added test TestSyncOnPrimitiveWrapperEvent.java 
>> to test for the JFR event case.
>> I tested the patch running tiers1-6 in mach5 with the flag set to 
>> DiagnoseSyncOnPrimitiveWrappers=2.
>
> Okay.
>
>> I checked it builds with arm32 and ppc but can't run any tests on 
>> those platforms, so it would be good if somebody can run the new test 
>> included in the patch.
>>
>> Let me know if you think I should run or add any more tests.
>
> The primary testing here is the new tests.
Here is v2:

Full: http://cr.openjdk.java.net/~pchilanomate/8242263/v2/webrev/
Inc: http://cr.openjdk.java.net/~pchilanomate/8242263/v2/inc/webrev/

Thanks David!

Patricio
> Thanks,
> David
> -----
>
>> Thanks!
>>
>> Patricio