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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20180411/784fcbb2/attachment-0001.html>


More information about the hotspot-gc-dev mailing list