RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to current_end
Stefan Johansson
stefan.johansson at oracle.com
Fri Apr 13 09:37:02 UTC 2018
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> 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