RFR(S): 8210513: Obsolete SyncFlags

[David Holmes]

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.

Thanks,
David

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