RFR(M): 8210848: Obsolete SyncKnobs
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Sep 21 21:25:52 UTC 2018
> New webrev:
>
>
http://cr.openjdk.java.net/~mikael/webrevs/8210848/webrev.02/syncknobs-full/open/webrev/
src/hotspot/share/opto/library_call.cpp
No comments.
src/hotspot/share/runtime/arguments.cpp
No comments.
src/hotspot/share/runtime/globals.hpp
No comments.
src/hotspot/share/runtime/objectMonitor.cpp
Wow. What a nightmare teasing that apart.
src/hotspot/share/runtime/objectMonitor.hpp
No comments.
src/hotspot/share/runtime/synchronizer.cpp
old L1799: if (ObjectMonitor::Knob_VerifyInUse) {
old L1800: verifyInUse(thread);
old L1801: }
The VerifyInUse support was useful in diagnosing bugs
in the monitor list stuff. I guess if we need it again,
we can pull it from the history and reapply/adapt it
to the current code.
old L1829: if (mid->owner() == THREAD) {
old L1830: if (ObjectMonitor::Knob_VerifyMatch != 0) {
old L1831: ResourceMark rm;
old L1832: Handle obj(THREAD, (oop) mid->object());
old L1833: tty->print("INFO: unexpected locked object:");
old L1834: javaVFrame::print_locked_object_class_name(tty, obj,
"locked");
old L1835: fatal("exiting JavaThread=" INTPTR_FORMAT
old L1836: " unexpectedly owns ObjectMonitor="
INTPTR_FORMAT,
old L1837: p2i(THREAD), p2i(mid));
old L1838: }
The VerifyMatch code was very useful in debugging locked
monitors where the unlock was somehow lost (opto bug,
lock elision bug, etc). I guess if we need it again,
we can pull it from the history and reapply/adapt it
to the current code.
src/hotspot/share/runtime/synchronizer.hpp
No comments.
src/hotspot/share/runtime/thread.cpp
old L1956: // Optionally release any monitors for regular
JavaThread exits. This
old L1957: // is provided as a work around for any bugs in
monitor enter-exit
old L1958: // matching. This can be expensive so it is not
enabled by default.
old L1959: //
old L1960: // ensure_join() ignores IllegalThreadStateExceptions,
and so does
old L1961: // ObjectSynchronizer::release_monitors_owned_by_thread().
old L1962: if (exit_type == jni_detach ||
ObjectMonitor::Knob_ExitRelease) {
The ExitRelease code was provided as a possible work around
for customers impacted by a lost monitor unlock. When a monitor
is lost and the owning thread has exited, no other thread can
enter that object monitor ever again which causes the application
to deadlock. The only recourse without the ExitRelease code is
to reboot the app.
I guess if we need it again, we can pull it from the history and
reapply/adapt it to the current code.
src/hotspot/share/runtime/vframe.cpp
L258: if (ObjectMonitor::Knob_Verbose && mark != NULL) {
L259: st->print("\t- lockbits=");
L260: mark->print_on(st);
L261: st->cr();
L262: }
This was the only way to see the lockbits in a thread dump.
Often useful when trying to debug a deadlock. Sigh...
So this looks like a clean removal of SyncKnobs so from that point
of view, thumbs up!
Dan
On 9/20/18 1:12 AM, Mikael Vidstedt wrote:
>
>> 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
More information about the hotspot-dev
mailing list