RFR(S): 8210513: Obsolete SyncFlags
david.holmes at oracle.com
Mon Sep 10 00:35:45 UTC 2018
On 8/09/2018 6:48 AM, Mikael Vidstedt wrote:
> Please review this change which obsoletes the SyncFlags flag.
> Bug: https://bugs.openjdk.java.net/browse/JDK-8210513 <https://bugs.openjdk.java.net/browse/JDK-8210513>
> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8210513/webrev.00/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8210513/webrev.00/open/webrev/>
> * Background (from bug)
> The experimental SyncFlags flag can in theory be used to tune how the synchronization primitives behave. The flag was convenient when the various implementations were compared a long time ago.
Actually it isn't a tuning flag it is a diagnostic flag. The different
values adjust part of the algorithm to try and detect the source of
bugs. I can't explain them all in great detail but ...
- 1: inject maximum blocking time to try to detect lost wakeup hangs
- 2: force current thread to take on responsible thread role if it's null
- 4: Don't null out responsible thread
- 8: inject extra fence after monitor entry
- 16: another variant of forcing current thread to take on responsible
- 32: inject extra fence() after monitor wait
> In practice the only implementation that is used and tested today is the default one. The SyncFlags flag no longer serves the purpose it used to, and is "Unsafe, Unstable" (the documentation of the flag says so explicitly). It should be obsoleted and later removed.
I generally agree that the flag is not needed with the stable
synchronization code that we have today. If the code were to be changed
in the future however similar constructs might be needed to diagnose any
bugs therein. Arguably if you know enough to touch this code then you
know enough to add back whatever diagnostics you may need. Though they
may also be useful for new ports, or even diagnosing memory barriers
bugs in new hardware.
So while the removal looks accurate I'm on the fence as to whether it
should be removed. Certainly if it were to stay then it needs
documenting and should probably be a real diagnostic flag.
Bottom line: code change is Reviewed. I won't fight for this code to
stay but wouldn't be upset if it did.
> Testing: I’m running the normal tier1 testing (still in progress).
More information about the hotspot-runtime-dev