RFR(M): 8210848: Obsolete SyncKnobs

David Holmes david.holmes at oracle.com
Fri Sep 21 21:43:42 UTC 2018 

Hi Mikael,

On 20/09/2018 1:12 AM, Mikael Vidstedt wrote:
> 
> 
>> On Sep 18, 2018, at 11:33 PM, David Holmes <david.holmes at oracle.com 
>> <mailto: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'll refrain from further comment on this. :)

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

I would think whomever is working on the monitor deflation problem might 
be interested in how ForceMonitorScavenge works. But I guess they can 
add something back if needed.

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

I see Dan answered this. Note sure the tests he refers to are valid, on 
the other hand such usage does seem to fall into a spec hole. Normal 
Java lock usage can't forget to release monitors. JNI attached threads 
that don't release, have monitors released when they detach. But Java 
threads that use JNI to lock but not unlock are not covered. okay forget 
about the assert.

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

Ok

>>
>> ---
>>
>> 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/%7Emikael/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/%7Emikael/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/%7Emikael/webrevs/8210848/webrev.02/syncknobs-20.2-Knob_ExitPolicy/open/webrev/>

No incremental for verifyInUse :(

I looked at the full webrev. All seems okay.

Thanks,
David

> 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/%7Emikael/webrevs/8210848/webrev.00/syncknobs-full/open/webrev/> 
>>> <http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.00/syncknobs-full/open/webrev/ 
>>> <http://cr.openjdk.java.net/%7Emikael/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/%7Emikael/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