RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to current_end

Tue Apr 10 22:21:08 UTC 2018

Hi Karen and Stefan,

@Stefan: Naming is hard :)
@Karen: thanks for the Graal comment, I fixed it in the new webrev, let me
know what you think :)

I think the naming convention suggested in this webrev came from
conversations in for JEP-331 and I am actually relatively indifferent to
the final result (as long as we have some form of forward progress :)). To
be honest, I'd also be happy to just leave _end as is for all architectures
and Graal and have a new _allocation_end. However, during initial reviews
of JEP-331 it was deemed complicated to understand:

_end -> the _end or sampling end
_allocation_end -> end pointer for the last possible allocation
hard_end -> allocation end + reserved space

That is how this naming came up and why hard_end went to "reserved_end".

I'm really indifferent, so I offer as a perusal:
http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.01/

I noticed a few problems of alignement of '{' so I'll go fix that.
Basically this webrev does the following:

- Uses fast_path_end instead of end
- Reverts hard_end back to where it was
- Adds the changes to Graal; there is a bunch of changes in Graal because
Graal still contains a bit of code doing fasttlabrefills.

Let me know what you think!

Thanks,
Jc

On Tue, Apr 10, 2018 at 6:56 AM Karen Kinnear <karen.kinnear at oracle.com>
wrote:

> Hi JC,
>
> A comment about Graal. The impact on Graal for this particular change
> would be to break it - so you’ll need
> to complete the Graal changes for this renaming. That isn’t optional or
> something that could be a follow-on. It
> is not ok to break a feature, even an experimental one. We will discuss in
> the other thread potential phasing of adding sampling.
>
> I did not do a thorough search -you can do that - I did find
> src/jdk.internal.vm.compiler/share/classes/
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   public final int threadTlabOffset = getFieldOffset("Thread::_tlab",
> Integer.class, "ThreadLocalAllocBuffer");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferStartOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_start", Integer.class,
> "HeapWord*");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferEndOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_end", Integer.class, "HeapWord*");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferTopOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_top", Integer.class, "HeapWord*");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferPfTopOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_pf_top", Integer.class,
> "HeapWord*");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferSlowAllocationsOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_slow_allocations", Integer.class,
> "unsigned");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferFastRefillWasteOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_fast_refill_waste", Integer.class,
> "unsigned");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferNumberOfRefillsOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_number_of_refills", Integer.class,
> "unsigned");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferRefillWasteLimitOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_refill_waste_limit",
> Integer.class, "size_t");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   private final int threadLocalAllocBufferDesiredSizeOffset =
> getFieldOffset("ThreadLocalAllocBuffer::_desired_size", Integer.class,
> "size_t");
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:
>   public final int tlabAlignmentReserve =
> getFieldValue("CompilerToVM::Data::ThreadLocalAllocBuffer_alignment_reserve",
> Integer.class, "size_t”);
>
> hope this helps,
> Karen
>
> On Apr 10, 2018, at 7:04 AM, Stefan Johansson <stefan.johansson at oracle.com>
> wrote:
>
> Hi JC,
>
> I realize that the names have been discussed before but I'm leaning
> towards suggesting something new. We discussed this briefly here in the
> office and others might have different opinions. One point that came up is
> that if we do this change and change all usages of
> JavaThread::tlab_end_offset() it would be good to make sure the new name is
> good. To us _current_end is not very descriptive, but naming is hard and
> the best we could come up with is _fast_path_end which would give the code:
>  cmpptr(end, Address(thread, JavaThread::tlab_fast_path_end_offset()));
>  jcc(Assembler::above, slow_case);
>
> I think this reads pretty good and is fairly clear. If we do this rename I
> think you can re-use _end in JEP-331 instead of calling it _allocation_end.
> But that is a later review :)
>
> Also, is there a good reason for renaming hard_end() to reserved_end()?
>
> One additional thing, you need to update the SA to reflect this change. I
> think the only place you need to fix is in:
>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ThreadLocalAllocBuffer.java
>
> Thanks,
> Stefan
>
> On 2018-04-09 19:24, JC Beyler wrote:
>
> Hi all,
> Small pre-amble to this request:
> In my work to try to get a heap sampler in OpenJDK (via JEP 331 <
> https://bugs.openjdk.java.net/browse/JDK-8171119>), I'm trying to reduce
> the footprint of my change so that the integration can be easier. I was
> told that generally a JEP webrev should be feature complete and go in
> at-once. However, with the change touching quite a bit of various code
> pieces, I was trying to figure out what could be separated as not "part of
> the feature".
> I asked around and said that perhaps a solution would be to cut up the
> renaming of TLAB's end field that I do in that webrev. Because I'm renaming
> a field in TLAB used by various backends for that work, I have to update
> every architecture dependent code to reflect it.
> I entirely understand that perhaps this is not in the habits and very
> potentially might not be the way things are generally done. If so, I
> apologize and let me know if you would not want this to go in separately :)
> Final note: there is still a chance JEP-331 does not go in. If it does
> not, we can leave the new name in place or I'll happily revert it. I can
> even create an issue to track this if that makes it easier for all.
> End of the pre-amble.
> The 33-line change webrev in question is here:
> http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.00/
> I fixed all the architectures and JVMCI and ran a few sanity tests to
> ensure I had not missed anything.
> Thanks for your help and I hope this is not too much trouble,
> Jc
> Ps: there is a graal change that needs to happen but I was not sure
> who/where <https://teams.googleplex.com/u/where> to ask about it. I was
> told it could happen in a separate webrev. Can anyone point me to the right
> direction? Should it just be hotspot-compiler-dev?
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180410/e655faf2/attachment.htm>