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

Fri Apr 10 17:19:27 UTC 2015

Dan,

Many thanks. The code changes look good.

Some questions/comments:

1. synchronizer.cpp: 
line 1727 - would you mind removing // TODO KK - that was an internal note to myself
to test that case - which I did :-) - but I forgot to remove the comment

1. macroAssembler_sparc/x86

From your response to David Holmes:
> - everything else is just experimental (did you actually test to any great extent with all these different possible EmitSync values?).

I've done minimal testing of the experimental code paths mostly
just to satisfy my "what the heck does this do?" curiosity...

In the past, Karen has done more experiments with the alternate
code paths, but my focus is on the OOTB config. Of course, Dave
Dice and Doug Lea use these experimental flags quite a bit as
part of their research work on locking. Dice has often used them
in customer situations to diagnose a system's Java monitor usage
and behavior patterns.

-- 
True confession - I did not shake out any of the EmitSync & XX code paths. Yes, Dice
and Doug Lea use them for research and Dice has used them for customer situations
to diagnose unusual behavior.

So I suspect you have tested them more thoroughly than I did. And yes, at
some point we may want to clean those up in consultation with Dice.

I do agree with removing the mfence and the Thread Tself. 

thanks,
Karen

On Apr 9, 2015, at 4:57 PM, 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
> - added and/or made comments consistent in synchronizer.[ch]pp
> - made phrase "in-use" consistent in comments and asserts
> 
> 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
>> 
>> 
> 