RFR 8242263: Diagnose synchronization on primitive wrappers

[David Holmes]

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.

> 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. 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?

---

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)

---

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;


  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?

---

test/hotspot/jtreg/runtime/Monitor/SyncOnPrimitiveWrapperTest.java

Please add "@bug 8242263".

  32  * @run main/othervm SyncOnPrimitiveWrapperTest

As the test only launches other VMs this can just be:

  32  * @run driver SyncOnPrimitiveWrapperTest

It would be nice to factor out the common options rather than repeating 
so many of the string literals.

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.

Both the FatalTest and the LogTest should be testing wrappers of each type.

  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.

101                 throw new RuntimeException("synchronization on 
primitve wrapper did not failed");

typos: primitve -> primitive
        failed -> fail

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.

---

  test/jdk/jdk/jfr/event/runtime/TestSyncOnPrimitiveWrapperEvent.java

The licence header should not be the version with the classpath exception.

Please add "@bug 8242263".

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

> 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.

Thanks,
David
-----

> Thanks!
> 
> Patricio