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

[Daniel D. Daugherty]

On 3/13/15 8:05 AM, Aleksey Shipilev wrote:
> On 03/13/2015 07:50 AM, David Holmes wrote:
>>> - remove old mfence() support
>> So I had expected to see this change conditional on the runtime
>> processor type. But then I checked Dave's blog and saw this was
>> determined 5+ years ago (!!!) so those older processors are no longer a
>> consideration. I do hope the performance tests have been re-run since
>> then though ;-)
> MFENCE still has stronger ordering guarantees, and LOCK'ed instructions
> are faster for regular accesses. Note that we also observed the
> performance effects when doing blind "lock addl %(rsp), 0":
>    https://bugs.openjdk.java.net/browse/JDK-8050147
>    http://hg.openjdk.java.net/jdk9/hs-comp/hotspot/rev/51e474c55d20
>    http://shipilev.net/blog/2014/on-the-fence-with-dependencies/
>
> Thanks,
> -Aleksey.
>

I addressed this part of David H's comment in my reply to him:

 > I do hope the performance tests have been re-run since
 > then though ;-)

I should have been more clear with my summary line:

 > - remove old mfence() support

Here's the complete diff for that part of the code:

@@ -2124,16 +2109,12 @@
         // We currently use (3), although it's likely that switching to (2)
         // is correct for the future.

         movptr(Address(tmpReg, OM_OFFSET_NO_MONITOR_VALUE_TAG(owner)), 
NULL_WORD);
         if (os::is_MP()) {
-          if (VM_Version::supports_sse2() && 1 == FenceInstruction) {
-            mfence();
-          } else {
-            lock (); addptr(Address(rsp, 0), 0);
+         lock(); addptr(Address(rsp, 0), 0);
            }
-       }
         // Ratify _succ remains non-null
         cmpptr(Address(tmpReg, OM_OFFSET_NO_MONITOR_VALUE_TAG(succ)), 0);
         jccb  (Assembler::notZero, LSuccess);

         xorptr(boxReg, boxReg);                  // box is really EAX


We only used the mfence() instruction when the FenceInstruction
was enabled and VM_Version::supports_sse2() == true.

Here's the rgrep for FenceInstruction:

$ rgrep FenceInstruction src
src/cpu/x86/vm/globals_x86.hpp:  product(intx, FenceInstruction, 
0,                                        \
src/cpu/x86/vm/macroAssembler_x86.cpp:          if 
(VM_Version::supports_sse2() && 1 == FenceInstruction) {

so the option is disabled by default and by removing the fence()
in this bucket, I'm removing the last (and only?) use of the
option. I'm going to need a CCC request for removing the flag...
and I was hoping to get through this project without any CCC
requests...

The FenceInstruction option appeared here:

$ sp -r1.956.1.1 src/share/vm/runtime/globals.hpp
src/share/vm/runtime/SCCS/s.globals.hpp:

D 1.956.1.1 07/03/09 17:40:52 dice 3486 3482    00002/00003/03062
MRs:
COMMENTS:
6468954 : minor clean up

$ sccs sccsdiff -r1.956 -r1.956.1.1 src/share/vm/runtime/globals.hpp

------- globals.hpp -------
727c727
<   product(intx, EmitLFence, 
0,                                              \
---
 >   product(intx, FenceInstruction, 
0,                                        \
2720,2721c2720
<   product(bool, UseThreadPriorities, 
true,                                  \
<           "Use native thread 
priorities")                                 \
---
 >   product_pd(bool, UseThreadPriorities,  "Use native thread 
priorities")    \

so the option used to be named 'EmitLFence'. That option
was introduced by this delta:

$ sp -r1.904.2.1 src/share/vm/runtime/globals.hpp
src/share/vm/runtime/SCCS/s.globals.hpp:

D 1.904.2.1 05/11/19 16:17:32 dice 3379 3375    00003/00000/02794
MRs:
COMMENTS:
6320199: improve code density for inline synchronization operations

$ sccs sccsdiff -r1.904 -r1.904.2.1 src/share/vm/runtime/globals.hpp

------- globals.hpp -------
651a652,654
 >   product(intx, EmitLFence, 
0,                                              \
 >           "(Unsafe,Unstable) Experimental")

So the EmitLFence was introduced in JDK6-B70 and has
always been disabled by default. I think it's safe to
say that generating the mfence() has been out of the
default code base for a long time.

I'm planning to stick with the removal of the mfence()
and I'll also get rid of the FenceInstruction option.

BTW, here's the complete entry for the option:

src/cpu/x86/vm/globals_x86.hpp:

   product(intx, FenceInstruction, 
0,                                        \
           "(Unsafe,Unstable) 
Experimental")                                 \

Looks like I missed changing the option from product(...)
to experimental(...) back in the cleanup bucket...

David H and/or Aleksey, please let me know if either of you
has any objections to this resolution.

Dan