RFR(M): 8210848: Obsolete SyncKnobs

David Holmes david.holmes at oracle.com
Wed Sep 19 06:33:15 UTC 2018 

Hi Mikael,

Code review from 34,000ft :)

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?

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 :) ).

---

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.

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

---

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.)

---

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.

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
>

More information about the hotspot-dev mailing list