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

Tue Apr 10 22:56:22 UTC 2018

Small update:

Here is the fixed webrev for the '{' that were out of alignment. This
passed release build JTREG for hotspot/jtreg/compiler/jvmti (just to run
something as a smoke screen) and hotspot/jtreg/compiler/aot/ (to test
Graal).
http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.02/

Let me know what you think,
Jc

On Tue, Apr 10, 2018 at 3:21 PM JC Beyler <jcbeyler at google.com> wrote:

> 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/6d68cac4/attachment.htm>