RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end (original) (raw)

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:

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 fastpathend(), then isn't it logical to have fastpathstart() paired with it? ("start" is already overloaded, but nobody is suggesting adding allocationstart()/currentstart()/hardstart() 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 >>> > > allocationend. However, during initial reviews of >>> JEP-331 >>> > it was >>> > > deemed complicated to understand: >>> > > >>> > > end -> the end or sampling end >>> > > allocationend -> end pointer for the last possible >>> allocation >>> > > hardend -> allocation end + reserved space >>> > > >>> > > That is how this naming came up and why hardend went >> to >>> > "reservedend". >>> > > >>> > > 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 fastpathend instead of end >>> > > - Reverts hardend 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::pftop", >>> > Integer.class, >>> > > "HeapWord*"); >>> > > >>> > >>> >> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: >>> > > private final int >>> > threadLocalAllocBufferSlowAllocationsOffset >>> > > = >>> getFieldOffset("ThreadLocalAllocBuffer::slowallocations", >>> > > Integer.class, "unsigned"); >>> > > >>> > >>> >> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: >>> > > private final int >>> > threadLocalAllocBufferFastRefillWasteOffset >>> > > = >>> > getFieldOffset("ThreadLocalAllocBuffer::fastrefillwaste", >>> > > Integer.class, "unsigned"); >>> > > >>> > >>> >> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: >>> > > private final int >>> > threadLocalAllocBufferNumberOfRefillsOffset >>> > > = >>> > getFieldOffset("ThreadLocalAllocBuffer::numberofrefills", >>> > > Integer.class, "unsigned"); >>> > > >>> > >>> >> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: >>> > > private final int >>> > > threadLocalAllocBufferRefillWasteLimitOffset = >>> > > >>> getFieldOffset("ThreadLocalAllocBuffer::refillwastelimit", >>> > > Integer.class, "sizet"); >>> > > >>> > >>> >> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: >>> > > private final int >>> > threadLocalAllocBufferDesiredSizeOffset = >>> > > >>> getFieldOffset("ThreadLocalAllocBuffer::desiredsize", >>> > > Integer.class, "sizet"); >>> > > >>> > >>> >> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: >>> > > public final int tlabAlignmentReserve = >>> > > >>> > >>> >> getFieldValue("CompilerToVM::Data::ThreadLocalAllocBufferalignmentreserve", >>> > > Integer.class, "sizet”); >>> > > >>> > > 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::tlabendoffset() it >>> > >> would be good to make sure the new name is good. >>> To us >>> > >> currentend is not very descriptive, but naming >>> is hard and >>> > >> the best we could come up with is fastpathend >>> which would >>> > >> give the code: >>> > >> cmpptr(end, Address(thread, >>> > >> JavaThread::tlabfastpathendoffset())); >>> > >> jcc(Assembler::above, slowcase); >>> > >> >>> > >> 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 allocationend. But that is a later >>> review :) >>> > >> >>> > >> Also, is there a good reason for renaming >>> hardend() to >>> > >> reservedend()? >>> > >> >>> > >> 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