RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to current_end
JC Beyler
jcbeyler at google.com
Wed Apr 11 15:48:27 UTC 2018
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/
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> 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>> 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> <
> 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?
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180411/784fcbb2/attachment.htm>
More information about the hotspot-gc-dev
mailing list