RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
JC Beyler
jcbeyler at google.com
Mon Apr 16 20:43:33 UTC 2018
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