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

Tue Apr 14 02:00:16 UTC 2015

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?

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