RFR(M): 8210848: Obsolete SyncKnobs

Wed Sep 19 03:04:45 UTC 2018

Right, I think the verifyInUse can all go. New version:

http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.01/syncknobs-full/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.01/syncknobs-full/open/webrev/>

Incremental webrev:

http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.01/syncknobs-07.2-Knob_VerifyInUse/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.01/syncknobs-07.2-Knob_VerifyInUse/open/webrev/>

I agree that some of the Knob variables can be renamed now. As a matter of fact in an earlier version of the patch I had done that rename, but I decided to roll that back to avoid making the change more complex. I’ll do that as an immediate follow up.

Cheers,
Mikael

> On Sep 18, 2018, at 4:36 PM, coleen.phillimore at oracle.com wrote:
> 
> 
> 
> 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
>>> 
>> 
> 