RFR(M): 8210848: Obsolete SyncKnobs

[David Holmes]

On 18/09/2018 9:07 PM, Claes Redestad 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.

It had to be deferred to a point where os::is_MP() returned an accurate 
result (it returns true before the number of processors is initialized). 
As the comment says that could now be moved e.g. to os::init_2().

It also served to avoid calling os::is_MP() all the time in the spin 
code - though that no longer seems to be a concern. With my changes to 
always treat as MP everywhere except spin-loops, os::is_MP becomes 
trivial and could be inlined.

Cheers,
David

> /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
>>
>