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

[David Holmes]

Hi Dan,

Sorry for the delay getting back to this.

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

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