RFR 8242263: Diagnose synchronization on primitive wrappers
David Holmes
david.holmes at oracle.com
Mon Aug 3 06:12:29 UTC 2020
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
More information about the hotspot-runtime-dev
mailing list