RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
dean.long at oracle.com
dean.long at oracle.com
Mon Apr 16 22:29:11 UTC 2018
Hi JC. Others might disagree, but I would vote "no" on doing this
renaming now, before the JEP, and even in the context of the JEP, I
don't think renaming the field requires renaming all the uses. The
compiler code is only interested in the fast path, so it's implicitly
understood. Also, if there is a fast_path_end(), then isn't it logical
to have fast_path_start() paired with it? ("start" is already
overloaded, but nobody is suggesting adding
allocation_start()/current_start()/hard_start() are they?). My opinion
is that it's fine the way it is.
dl
On 4/16/18 1:43 PM, JC Beyler wrote:
> Hi all,
>
> I've left the mail thread from the hotspot-gc-dev below for tracking
> purposes but will start a new request here.
>
> - I'm trying to push a renaming of a TLAB field to make my work for JEP-331
> <http://openjdk.java.net/jeps/331> easier
> - There is an understanding that if JEP-331 does not make it, this might
> be useless and I'll revert if that is what the community wants; however the
> new name seems better anyway so perhaps not?
>
> - The webrev renames a field used across the various back-ends and Graal
> - The webrev is here:
> http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.04/
>
> Could I please get some feedback on this?
>
> Thanks all for your help,
> Jc
>
>
>
> On Fri, Apr 13, 2018 at 2:37 AM Stefan Johansson <
> stefan.johansson at oracle.com> wrote:
>
>> Hi JC,
>>
>> I don't have a name, but I've looked at bit more at the failures and I
>> think they are unrelated and one of the local compiler engineers agree.
>>
>> I also ran some local testing and was not able to get any error with you
>> latest changes, but verified that it doens't work without the graal
>> parts. So they seem good. It might still be good to switch this over to
>> the general hotspot-dev list to let someone with Graal knowledge to look
>> at the change and make sure everything is correct.
>>
>> Thanks,
>> Stefan
>>
>>
>> On 2018-04-12 17:26, JC Beyler wrote:
>>> Hi Stefan,
>>>
>>> Thanks for testing :). I've renamed the bug title in the JBS and will
>>> emit a new webrev shortly. Do you have the name of a compiler engineer
>>> in mind or should I perhaps now move this conversation to the general
>>> hotspot-dev mailing list and ask there?
>>>
>>> The alternative is to add the compiler-mailing list to this email thread
>>> and ask there before sending to the general list. What do you think is
>> best?
>>> Thanks for all your help,
>>> Jc
>>>
>>> On Thu, Apr 12, 2018 at 2:09 AM Stefan Johansson
>>> <stefan.johansson at oracle.com <mailto:stefan.johansson at oracle.com>>
>> wrote:
>>>
>>>
>>> On 2018-04-11 17:48, JC Beyler wrote:
>>> > Hi Stefan,
>>> >
>>> > Sorry about that, I must have missed adding the files or
>>> something. Here
>>> > is a webrev that added the changes for the SA file:
>>> > http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.03/
>>> >
>>> No problem, this looks good. I kicked of a run in our test system to
>>> get
>>> some more coverage and have tried to include some Graal testing. I'll
>>> let you know the results once they are done.
>>>
>>> Please also update the bug title both in JBS and the changeset.
>>>
>>> Cheers,
>>> Stefan
>>>
>>> > I changed the method name, which propagated a change to:
>>> >
>>>
>> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ObjectHeap.java
>>> >
>>> > I tried testing your test file. It exists in my branch (if the
>>> same) under:
>>> > hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java
>>> >
>>> > and interestingly (which generally means I did something wrong),
>> it
>>> > passed before/after the change so I could not verify that this is
>>> a test
>>> > showing that all is well in the world on my side. Any ideas of
>>> what I
>>> > did wrong?
>>> >
>>> > I did again test it for hotspot/jtreg/compiler/aot/ and
>>> > hotspot/jtreg/serviceability/jvmti and it passes those.
>>> >
>>> > Thanks for all your help,
>>> > Jc
>>> >
>>> >
>>> >
>>> > On Wed, Apr 11, 2018 at 7:44 AM Stefan Johansson
>>> > <stefan.johansson at oracle.com <mailto:stefan.johansson at oracle.com>
>>> <mailto:stefan.johansson at oracle.com
>>> <mailto:stefan.johansson at oracle.com>>> wrote:
>>> >
>>> > 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>
>>> > <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>>
>>> > > <mailto:jcbeyler at google.com <mailto:jcbeyler at google.com>
>>> <mailto: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> <mailto:karen.kinnear at oracle.com
>>> <mailto:karen.kinnear at oracle.com>>
>>> > <mailto:karen.kinnear at oracle.com
>>> <mailto:karen.kinnear at oracle.com> <mailto: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>
>>> > <mailto:stefan.johansson at oracle.com
>>> <mailto:stefan.johansson at oracle.com>>
>>> > >> <mailto:stefan.johansson at oracle.com
>>> <mailto:stefan.johansson at oracle.com>
>>> > <mailto: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>
>>> <https://teams.googleplex.com/u/where>
>>> > <https://teams.googleplex.com/u/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-dev
mailing list