RFR(S): 8210513: Obsolete SyncFlags

[Mikael Vidstedt]

> On Sep 9, 2018, at 5:35 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Mikael,
> 
> 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 thread role
> - 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.

Sorry, I couldn’t resist, change pushed :)

Thanks Coleen/Dan/David for the reviews!

Cheers,
Mikael

> 
> Thanks,
> David
> 
>> Testing: I’m running the normal tier1 testing (still in progress).
>> Cheers,
>> Mikael