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

Stefan Johansson stefan.johansson at oracle.com
Wed Apr 11 14:44:55 UTC 2018


Hi JC,

On 2018-04-11 00:56, JC Beyler wrote:
> 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/
> 
I think this looks better, I agree that leaving _end is tempting to 
avoid a lot of change, but I think this will be better in the long run.

I still miss the changes to make the SA work. To see a problem you can run:
make CONF=fast run-test 
TEST=open/test/hotspot/jtreg/serviceability/ClhsdbJhisto.java

Cheers,
Stefan

> Let me know what you think,
> Jc
> 
> On Tue, Apr 10, 2018 at 3:21 PM JC Beyler <jcbeyler at google.com 
> <mailto: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 <mailto: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
>>         <mailto: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?
> 



More information about the hotspot-gc-dev mailing list