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