JDK-7125896 : Eliminate nested locks
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Affected Version: 8-pool
  • Priority: P4
  • Status: Closed
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2011-12-29
  • Updated: 2024-01-24
  • Resolved: 2012-03-29
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 7 JDK 8 Other
7u4Fixed 8Fixed hs23Fixed
Related Reports
Relates :  
JDK-7128355 - assert(!nocreate) failed: Cannot build a phi for a block already parsed
Relates :  
JDK-7128352 - assert(obj_node == obj) failed
Relates :  
JDK-7129618 - assert(obj_node->eqv_uncast(obj),"");
Description
Eliminated nested locks of the same object:

    synchronized (reader) { // we synchronize on reader
    if (reader.getCount() > 0) { // calling synchronized method getCount() while already holding the lock
       reader.incrementReserveCount(1);
    }
Comments
Conversation with Tom R. during review: Tom: All locking in compiled code is required to be block structured so the slot of the BoxLockNodes must match if they feed into a Phi and GVN should common them up to be equivalent, so the Phi should just collapse. Me: You forgot that BoxLockNode commoning is switched off for EliminateNestedLocks, it is main reason for Phis. Phis are generated for monitorexit (unlocks) when monitorenter (locks) code is merged (as was in loop head cloning case and could be in other cases). Yes, in current code BoxLockNode are commoned with the same slot (but could be for different objects) so it could bei used by eliminated and not eliminated locks. That is why elimination code clones BoxLockNode only for eliminated locks. Because in current code the same BoxLockNode could be referenced by eliminated and not eliminated locks. Tom: That's true with commoning of BoxLocks enabled but with it turned off they should follow the block structure and could be eliminated and referenced as a group. I actually think not commoning the BoxLocks should be the only behaviour instead of making it conditional on EliminateNestedLocks, though I have a vague memory of some code that relies on commoning of them for some purpose. I can't seem to find it though. Me: No, Box and Lock nodes are different since they are generated on different paths and are not commoning (I had to change code in Parse::ensure_phi() for BoxLockNode). To be clear we are talking about this case: if (a) { monitorenter(obj) } else { monitorenter(obj) } monitorexit(obj) In above case you have several Box and Phi nodes associated with one locking region. So you can't mark only one Box node. Commoning simplified all compiler optimizations since you need only compare Box nodes. Now I have to look through Phi to find Box nodes and compare their slots. For example, look on lock coarsening code changes. Tom: I assumed we wouldn't compile things like that but I guess that's technically block structured. The monitor matching logic really accepts that as block structured? I think it should reject it to simplify the compiler. There's no benefit to accepting code like that since it could always be restructured to properly pair. Why do you have to compare their slots? As far as I can see, BoxLock is created for monitorenter and the unlock pulls the box from the JVMState, guaranteeing that a single BoxLock corresponds to a single lock region, which seems like a nice property. Am I missing something? I guess it's just the example above? Me: I mean, lock coarsening processes two lock regions which have separate Box nodes. So compared unlock from first region and lock from second region have different box nodes. Tom: Oh right. I don't actually think the compare is needed at all since if the locks are actually block structured then an unlock followed by a lock will always use the same slot. Even if they didn't it would still be a valid optimization. It's fine to leave it as it is though.
24-01-2024
EVALUATION http://hg.openjdk.java.net/lambda/lambda/hotspot/rev/e9a5e0a812c8
22-03-2012
EVALUATION http://hg.openjdk.java.net/hsx/hotspot-rt/hotspot/rev/e9a5e0a812c8
18-01-2012
EVALUATION Nested locks elimination done before lock nodes expansion by looking for outer locks of the same object. Commoning (GVN) of BoxLock nodes is switched off because nested locks elimination requires separate BoxLock node for each locked region to generated correct debug info for deoptimization. As result there could be merges (and Phi nodes) of BoxLock nodes. One such merge generated by ciTypeFlow (cloning loop head) is avoided but there could be other cases so new code is added to handle it. New code is under new product flag EliminateNestedLocks. Added new enum field AbstractLockNode::_kind (I tried to avoid _type name) and corresponding methods instead of boolean fields. Added new method LockNode::is_nested_lock_region() to find if it is nested. Added new method BoxLockNode::is_simple_lock_region() to check if Box node is used to lock only one object and to avoid replacing this Box with clone in macro.cpp. Added new method PhaseMacroExpand::mark_eliminated_box() to move EA eliminated marking code from mark_eliminated_locking_nodes(). Also added missed KILL effect for box register in fastlock and fastunlock mach nodes definitions.
09-01-2012
EVALUATION http://hg.openjdk.java.net/hsx/hotspot-comp/hotspot/rev/e9a5e0a812c8
07-01-2012