RFR(M): 8210848: Obsolete SyncKnobs

Thu Sep 20 05:12:38 UTC 2018

> On Sep 18, 2018, at 11:33 PM, David Holmes <david.holmes at oracle.com> wrote:
> 
> Hi Mikael,
> 
> Code review from 34,000ft :)

Ha! :)

> Generally looks good for the algorithmic selection knobs. Though I am sad to see some of this go - specifically the code for the different queuing policies after a notify(). I have vague recollections of applications encountering pathological behaviour with particular and excessive use of wait/notify, that was fixed by changing the queuing policy. But at least that code would be relative simple to add back if someone wanted to.
> 
> I have some concerns about the handling of the spin related knobs. We have sophisticated spin logic to optimise performance** which involves a large number of tuning knobs. Many of those knobs are gone as their values could only change at build time. A few remain because they are affected by os::is_MP. So what we end up with is a partial simplification where the code is no longer readily tunable (if someone needed to do it), but nor is it that simple.
> 
> ** What concerns me is that we have this sophisticated but unused code, not because it is stale/dead/un-needed, but because the collective "we" has lost the ability and/or motivation to actually try and tune this as hardware has evolved. We no longer know how optimal the remaining spin knobs actually are - and certainly not across the broader range of architectures that OpenJDK now supports. Maybe the better path for this "unused code" is to start using it?

Right, it all comes down to priorities and where we spend our time (no pun intended). Since nobody seems to have looked at/used/changed this recently I think we’ll have to assume and accept that this is not the area we consider in need of the most attention right now. Simplifying the code at least means we get rid of the unused complexity, and it’s easier for people to understand the part of the algorithm and code actually used today.

> I have a few specific comments:
> 
> Knob_Verbose: instead of outright deleting this now should we instead have a RFE to convert to UL where appropriate and feasible (and perhaps just use tty and TraceXXX if UL can not be used). In particular logging around ForceMonitorScavenge might be useful (unless we're going to obsolete that too :) ).

I’d be very hesitant to spend time on investigating the interaction with UL and/or introducing a trace flag unless the logging is actually useful, and I personally haven’t seen any evidence of that, at least not in modern times. Do speak up if you read this and have found it useful enough to motivate the investment!

> ---
> 
> thread.cpp:
> 
> This comment block is no longer relevant:
> 
> 1956   // Optionally release any monitors for regular JavaThread exits. This
> 1957   // is provided as a work around for any bugs in monitor enter-exit
> 1958   // matching. This can be expensive so it is not enabled by default.

Good catch, consider it removed!

> I wonder if we could just assert there are no held monitors in the regular JavaThread case … ?

Good question, I don’t have an answer :)

> 
> ---
> 
> synchronizer.cpp:
> 
> is verifyInUse dead code now? (I have to wonder whether everything would be shown to be okay if we turned on these checks? More generally is this a sanity check we should actually be performing in debug builds, or was this really only useful during the development of the "in use" lists? I suspect the latter.)

Yes, Coleen also found that verifyInUse is indeed dead - consider it gone (in webrev.01).

> 
> ---
> 
> objectMonitor.cpp:
> 
> Aside: This comment block is out of date and can be deleted.
> 
> 904     // I'd like to write one of the following:
> 905     // A.  OrderAccess::release() ; _owner = NULL
> 906     // B.  OrderAccess::loadstore(); OrderAccess::storestore(); _owner = NULL;
> 907     // Unfortunately OrderAccess::release() and OrderAccess::loadstore() both
> 908     // store into a _dummy variable.  That store is not needed, but can result
> 909     // in massive wasteful coherency traffic on classic SMP systems.
> 910     // Instead, I use release_store(), which is implemented as just a simple
> 911     // ST on x64, x86 and SPARC.

Consider it removed!

New webrev:

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

Incremental changes from webrev.01:

http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.02/syncknobs-04.2-Knob_ExitRelease/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.02/syncknobs-04.2-Knob_ExitRelease/open/webrev/>
http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.02/syncknobs-20.2-Knob_ExitPolicy/open/webrev/ <http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.02/syncknobs-20.2-Knob_ExitPolicy/open/webrev/>

Cheers,
Mikael

> 
> Thanks,
> David
> 
> On 18/09/2018 5:03 PM, 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