RFR(M): 8210848: Obsolete SyncKnobs

[Mikael Vidstedt]

Thanks for the review, and very good point about DeferredInitialize. I have a patch ready which performs the initialization eagerly at startup, but I’ll do that as a follow up to avoid making this particular change and review more complex.

Cheers,
Mikael

> On Sep 18, 2018, at 4:07 AM, Claes Redestad <claes.redestad at oracle.com> wrote:
> 
> Great cleanup!
> 
> src/hotspot/share/runtime/objectMonitor.cpp:
> 
>   The DeferredInitialize method seems a bit silly now that it only guards the setting of three little Knobs using a CAS.. perhaps we would be better off wrapping these three in methods (spinLimit() { return os::is_MP() ? 5000 : 0; }), or perhaps we can guarantee that os::is_MP is initialized before, somehow? Either way I think we could profitably get rid of DeferredInitialize and InitDone.
> 
> /Claes
> 
> On 2018-09-18 09:03, Mikael Vidstedt wrote:
>> Please review this change which obsoletes the SyncKnobs flag, and simplifies the code by removing the resulting unused code paths.
>> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8210848
>> Webrev: http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-full/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-full/open/webrev/>
>> 
>> * Background (from the bug)
>> 
>> The experimental SyncKnobs flag can in theory be used to tune the behavior of the synchronization primitives. The flag was convenient when the various algorithms were compared a long time ago.
>> 
>> In practice the only algorithm used and tested today are the default one. The SyncKnobs flag no longer serves the purpose it used to, and is "Unstable" (the documentation of the flag says so explicitly). It should be obsoleted and later removed.
>> 
>> * About the change
>> 
>> As mentioned above this change not only obsolete the SyncKnobs flag, but also removes the various sub-options/knobs and prunes the resulting dead code. Of course I’m happy to hear your thoughts on that. After having asked around it seems like the knobs have not been used the last decade or so, and I like to think that the synchronization code is complex enough without what is in that case effectively dead code.
>> 
>> In order to make reviewing this slightly easier I have for your convenience (at least I hope it is) also created a bunch of smaller patches which stack, each removing one specific knob. Hopefully they’re more or less self-explanatory, but let me know if I can help clarify something:
>> 
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-00-base/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-01-Knob_ReportSettings/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-02-Knob_SpinBackOff/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-03-BackOffMask/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-04-Knob_ExitRelease/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-05-Knob_InlineNotify/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-06-Knob_Verbose/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-07-Knob_VerifyInUse/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-08-Knob_VerifyMatch/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-09-Knob_SpinBase/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-10-Knob_CASPenalty/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-11-Knob_OXPenalty/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-12-Knob_SpinSetSucc/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-13-Knob_SpinEarly/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-14-Knob_SuccEnabled/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-15-Knob_SuccRestrict/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-16-Knob_MaxSpinners/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-17-Knob_SpinAfterFutile/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-18-Knob_OState/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-19-Knob_UsePause/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-20-Knob_ExitPolicy/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-21-Knob_ResetEvent/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-22-Knob_FastHSSEC/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-23-Knob_MoveNotifyee/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-24-Knob_QMode/open/webrev/
>> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-25-Obsolete/open/webrev/
>> 
>> * Testing
>> 
>> A slightly earlier version of this patch passed the standard tier1-3 testing. I will run additional testing after I get some feedback.
>> 
>> Cheers,
>> Mikael
>> 
>