RFR(M): 8210848: Obsolete SyncKnobs

Tue Sep 18 23:36:46 UTC 2018

On 9/18/18 6:36 PM, coleen.phillimore at oracle.com wrote:
>
> This looks like a nice cleanup!
>
> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-full/open/webrev/src/hotspot/share/runtime/synchronizer.cpp.frames.html 
>
>
> verifyInUse can be removed also.
>
> Also, the verification removed at line 1830 seems like it could be 
> useful.  Can you make it an assert instead?  Sorry this might 
> invalidate your testing.

I read this too fast.  This doesn't verify anything.  I has a fatal 
error if you own any locks in this ReleaseJavaMonitorsClosure.  Not sure 
if that's a problem or not but we don't want to start giving fatal 
errors for it unconditionally like this.

Coleen
>
> http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-full/open/webrev/src/hotspot/share/runtime/objectMonitor.cpp.frames.html 
>
>
> There are still some variables called Knob_Something that should 
> probably be renamed to something meaningful, but could be done in a 
> future RFE and not make this change larger.
>
> That's all I saw.  Looks good!
> Coleen
>
> On 9/18/18 3:03 AM, 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
>>
>