RFR(S) Contended Locking fast exit bucket (8073165)

Tue Apr 14 02:06:50 UTC 2015

On 14/04/2015 12:00 PM, Daniel D. Daugherty wrote:
> On 4/13/15 7:22 PM, David Holmes wrote:
>> Hi Dan,
>>
>> Sorry for the delay getting back to this.
>
> Not a problem... still gathering all the updated perf
> test results...
>
>
>> On 10/04/2015 6:57 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have the revised Contended Locking fast exit bucket ready for review.
>>>
>>> This work is being tracked by the following bug ID:
>>>
>>>      JDK-8073165 Contended Locking fast exit bucket
>>>      https://bugs.openjdk.java.net/browse/JDK-8073165
>>>
>>> Here is the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8073165-webrev/1-jdk9-hs-rt/
>>>
>>> Here is the JEP link:
>>>
>>>      https://bugs.openjdk.java.net/browse/JDK-8046133
>>>
>>> Thanks to David H, Aleksey S and Coleen for their comments!
>>>
>>> The best way to review this round is to download the two patch files:
>>>
>>> Summary of changes:    284 lines changed: 163 ins; 51 del; 70 mod; 34386
>>> unchg
>>> http://cr.openjdk.java.net/~dcubed/8073165-webrev/0-jdk9-hs-rt/hotspot.patch
>>>
>>>
>>>
>>> Summary of changes:    356 lines changed: 166 ins; 55 del; 135 mod;
>>> 32524 unchg
>>> http://cr.openjdk.java.net/~dcubed/8073165-webrev/1-jdk9-hs-rt/hotspot.patch
>>>
>>>
>>>
>>> and view them in your favorite file merge tool.
>>>
>>> Here are the changes from Code Review Round 0:
>>>
>>> - The changes to src/share/vm/runtime/thread.hpp have been
>>>    backed out (no longer adding a _TSelf field); removed the
>>>    (EmitSync & 32) optimization that used the _TSelf field
>>> - merge the SPARC and X86 comments for the (EmitSync & 1024)
>>>    optimization and apply to both
>>> - remove the FenceInstruction option; still need to file a
>>>    fast-track CCC request
>>> - cleaned up various comments
>>> - cleaned up variable naming in synchronizer.[ch]pp
>>>    - global variables now consistently begin with a 'g'
>>>      like gCamelCaseGlobalVariable
>>>    - locals like 'inusetally' are renamed to 'in_use_tally'
>>>    - made functions with a mix of lowercase and camelCase
>>>      parameters consistently camelCase
>>
>> Great cleanup! You only seem to have overlooked
>> ObjectSynchronizer::omFlush which still has locals starting with caps
>> and names like InUseTally and InUseList. :)
>
> I'll look at cleaning that one up also. Is it safe to presume
> that such a cleanup doesn't require another webrev?

Safe assumption on my part :)

Thanks,
David

>
>>
>>> - added and/or made comments consistent in synchronizer.[ch]pp
>>> - made phrase "in-use" consistent in comments and asserts
>>
>> All looks good - nothing further from me.
>
> Thanks for the reviews!
>
> Dan
>
>
>>
>> Thanks,
>> David
>>
>>> Dan
>>>
>>>
>>> On 3/9/15 11:11 AM, Daniel D. Daugherty wrote:
>>>> Greetings,
>>>>
>>>> I have the Contended Locking fast exit bucket ready for review.
>>>>
>>>> The code changes in this bucket are primarily assembly language
>>>> tweaks that come into play when an inflated ObjectMonitor is in
>>>> use. There are tweaks that apply to the default code path and
>>>> there are tweaks that only apply when an experimental flag is
>>>> set. This code code review is focused on the default code path
>>>> or the "out of the box" configuration.
>>>>
>>>> This work is being tracked by the following bug ID:
>>>>
>>>>     JDK-8073165 Contended Locking fast exit bucket
>>>>     https://bugs.openjdk.java.net/browse/JDK-8073165
>>>>
>>>> Here is the webrev URL:
>>>>
>>>> http://cr.openjdk.java.net/~dcubed/8073165-webrev/0-jdk9-hs-rt/
>>>>
>>>> Here is the JEP link:
>>>>
>>>>     https://bugs.openjdk.java.net/browse/JDK-8046133
>>>>
>>>> Testing:
>>>>
>>>> - Aurora Adhoc RT/SVC baseline batch
>>>> - JPRT test jobs
>>>> - MonitorExitStresser micro-benchmark (in process)
>>>> - MonitorEnterExitStresser micro-benchmark (in process)
>>>> - CallTimerGrid stress testing (in process)
>>>> - Aurora performance testing:
>>>>   - out of the box for the "promotion" and 32-bit server configs
>>>>   - heavy weight monitors for the "promotion" and 32-bit server configs
>>>>     (-XX:-UseBiasedLocking -XX:+UseHeavyMonitors)
>>>>     (in process)
>>>>
>>>>
>>>> 8073165 summary of changes:
>>>>
>>>> macroAssembler_sparc.cpp: MacroAssembler::compiler_unlock_object()
>>>>
>>>> - default (OOTB) EmitSync value == 0
>>>> - an _owner field optimization is in the non-default path under
>>>>   a (EmitSync & 1024) check
>>>>
>>>> - classic lock release without 1-0 support is in the non-default
>>>>   path under a (EmitSync & 512) check
>>>>
>>>> - an EntryList, cxq and _succ field optimization is in the
>>>>   non-default path under a (EmitSync & 16384) check
>>>>
>>>> - the default path is now the 1-0 form that avoids CAS and membar
>>>>   in the common case; see the "1-0 exit" section in objectMonitor.cpp
>>>>
>>>> sharedRuntime_sparc.cpp:
>>>>
>>>> - add JavaThread* param for
>>>>   SharedRuntime::complete_monitor_unlocking_C call
>>>>
>>>> macroAssembler_x86.cpp: MacroAssembler::fast_unlock()
>>>>
>>>> - with all the new/modified comments and all the optional
>>>>   code path updates, it is hard to see that the only real
>>>>   changes to the default code path are 1) the _owner==Self
>>>>   check has been made optional and 2) the optional succ
>>>>   field check for the slow path is now on the default path
>>>>
>>>> - default (OOTB) EmitSync value == 0
>>>> - remove optional (EmitSync & 8) code
>>>> - refactor code in MacroAssembler::fast_unlock
>>>> - remove old mfence() support
>>>> - an _owner field optimization is in the non-default path under
>>>>   a (EmitSync & 1024) check
>>>> - an _owner field optimization is in the non-default path under
>>>>   a (EmitSync & 16) check
>>>> - a barrier optimization is in the non-default path under a
>>>>   (EmitSync & 32) check
>>>> - remove old AMD optional work around
>>>>
>>>> sharedRuntime_x86_32.cpp:
>>>> sharedRuntime_x86_64.cpp:
>>>>
>>>> - add thread param for SharedRuntime::complete_monitor_unlocking_C
>>>>   call
>>>>
>>>> macro.[ch]pp: PhaseMacroExpand::make_slow_call()
>>>>
>>>> - add param2 and update callers
>>>>
>>>> runtime.[ch]pp: OptoRuntime::complete_monitor_exit_Type()
>>>>
>>>> - add JavaThread* parameter
>>>>
>>>> sharedRuntime.[ch]pp: SharedRuntime::complete_monitor_unlocking_C()
>>>>
>>>> - add JavaThread* parameter
>>>>
>>>> synchronizer.cpp: ObjectSynchronizer::omRelease()
>>>>
>>>> - cleanup in use list traversal
>>>> - add assert that ObjectMonitor being released was
>>>>   actually found and extracted
>>>>
>>>> synchronizer.[ch]pp:
>>>>
>>>> - add some comments in various places
>>>> - rename walk_monitor_list() -> deflate_monitor_list()
>>>>   and update callers
>>>> - minor logic refactoring
>>>>
>>>> thread.hpp:
>>>>
>>>> - add _TSelf field for mfence() replacement
>>>>
>>>>
>>>> Thanks, in advance, for any comments, questions or suggestions.
>>>>
>>>> Dan
>>>>
>>>>
>>>
>