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

Fri Apr 10 20:24:06 UTC 2015

Karen,

Thanks for the fast review!  As usual, replies are embedded below...

On 4/10/15 11:19 AM, Karen Kinnear wrote:
> Dan,
>
> Many thanks. The code changes look good.

Thanks!

As usual for the Contended Locking project, this changeset will be:

Contributed-by: dave.dice at oracle.com, karen.kinnear at oracle.com, 
daniel.daugherty at oracle.com

> 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

Hmmm... Found a 'TODO KK' on old line 1472 and new line 1491
and deleted it. Didn't find any others.

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

Interesting... I could swear that some of the scripts
that you gave me had '-XX:EmitSync=...' options in them...
No matter for the purposes of this project...

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

We definitely want to the clean them up.

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

Thanks!

Dan

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