RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
JC Beyler
jcbeyler at google.com
Mon Apr 16 23:42:38 UTC 2018
Hi Dean,
I think perhaps this is also an attempt to figure out the naming of all
this because naming (or renaming like here) is often the start of big
conversations :).
Originally, in the JEP-331 work, I had left the _end field as is and, by
doing so, this whole renaming webrev goes away. However, if we do that,
then the TLAB code contains:
_end: the fast path end, can be the allocation end or the to-be-sampled end
_allocation_end: the actual allocation end of the current TLAB
hard_end: _allocation_end + reserved space
With an early iteration of a webrev for JEP-331, a conversation occurred
about whether or not that was clear or not and it was determined that this
renaming was more clear:
_current_end: the fast path end
_allocation_end: the actual allocation end of the current TLAB
reserved_end: _allocation_end + reserved space
Because I'm trying to reduce the footprint of files changed, I pulled out
the renaming changes into this webrev. While talking about it with the GC
team, the renaming suggested became:
_fast_path_end: the fast path end
_allocation_end: the actual allocation end of the current TLAB
hard_end: _allocation_end + reserved space
Now, to be honest, any renaming or no renaming is fine by me. I like not
renaming the fields to not change the code of every backend and Graal; I
also like the fast_path_end rename due to it making the backend code easy
to read as mentioned in the GC mailing lis thread.
So there are two questions really:
- Should we rename the _end field and thus provoke the changes in this
webrev?
- If we do want to change the field, should/could it go in an initial
webrev or should it go in the JEP-331 webrev whenever/ifever it goes in?
And if we do wait, could we at least converge on a renaming now so that
this does not become a point of contention for that webrev's review?
If I read your answer correctly:
- You are saying that we should keep the _end field altogether; or at
least only rename the field not the methods to access it, thus reducing the
change scope.
- You are also saying to wait for the JEP-331 webrev's final iteration
and integrate it in one step
Have I understood your answer correctly?
Again, I am fine with renaming to whatever or not renaming at all. I just
am trying to get forward progress here :)
Thanks for your help!
Jc
On Mon, Apr 16, 2018 at 3:29 PM <dean.long at oracle.com> wrote:
> 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>
> >>> > <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