RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end (original) (raw)
Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Apr 18 22:06:42 UTC 2018
- Previous message: RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
- Next message: RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On 4/18/18 2:54 PM, JC Beyler wrote:
Seems like there is no consensus really except that changing it has a few headaches involved. Should we then do the implementation of "8171119: Low-Overhead HeapProfiling" without changing the end field and postpone this conversation to afterwards when Graal handles multi-release in a more mature manner (read has been doing it for a while).
I would prefer this way.
Thanks, Vladimir
Or is there still a path forward? (Re)Naming is hard, this is proof of that. That puts us back to what I had originally: end: the fast path end, can be the allocation end or the to-be-sampled end allocationend: the actual allocation end of the current TLAB hardend: allocationend + reserved space Thanks for all of your input and help on this, Jc On Wed, Apr 18, 2018 at 12:46 PM Vladimir Kozlov <vladimir.kozlov at oracle.com_ _<mailto:vladimir.kozlov at oracle.com>> wrote: On 4/18/18 12:15 PM, dean.long at oracle.com <mailto:dean.long at oracle.com> wrote: > I will defer to Vladimir, but I would have been happy with something like: > > // preserve simple start/end abstraction for compiler > HeapWord* end() const { return fastpathend(); } > static ByteSize endoffset() { return fastpathendoffset(); } This is ugly. > > even though endoffset() would then refer to a virtual "end" field. > > Vladimir, are you also approving the Graal changes? :-) You really made my day :( S..t! We can't make this change in Graal as suggested because we will break Graal's JDK 8 support. Graal has direct access to VM's fields through JVMCI. You have to guard change with JDK version check. Labs start addressing multi-releases so it may be not big issue anymore. See Doug's comment for 8201318 RFR. Anyway you have to file new PR (pull request) for Graal changes on https://github.com/graalvm/mx/pulls. And I am not sponsoring this. I don't think such renaming worse all our efforts. Good luck, Vladimir > > dl > > On 4/18/18 11:02 AM, Vladimir Kozlov wrote: >> Ganging up on us ;) >> >> Yes, I missed original discussion about renaming on GC list. >> >> From my point of view next code looks better because it seems compare related values: >> >> cmpptr(end, Address(thread, JavaThread::tlabendoffset())); >> >> But I would not strongly object renaming to move this JEP forward. Consider changes reviewed and approved by me. >> >> Regards, >> Vladimir >> >> On 4/18/18 6:29 AM, Daniel D. Daugherty wrote: >>> Greetings, >>> >>> The idea of splitting this change off from "8171119: Low-Overhead Heap Profiling" >>> came up during the design review. It might have been me that suggested the split >>> off or it was someone else. Sorry I don't remember. >>> >>> In any case, the rename of "end" to "fastpathend" is just a clarity change to >>> the existing code and I think that change can easily stand on its own. That is >>> particularly true if the accessors are renamed at the same time. I think having >>> the accessor names match the field names is a pretty well known HotSpot rule >>> so I'm not sure why we're talking about not renaming the accessors... >>> >>> Personally, I prefer "fastpathend" over "currentend". >>> >>> Dan >>> >>> >>> On 4/18/18 4:04 AM, Stefan Johansson wrote: >>>> Hi, >>>> >>>> On 2018-04-18 02:02, Vladimir Kozlov wrote: >>>>> > I think I like better not to add it. If no one is using it, there should be >>>>> > no reason to add it? By not adding it, it also makes any future wish to >>>>> > have it a nice indicator of: hey why do you want to see this? Same as >>>>> > hardend btw which is not visible. Am I missing something? >>>>> >>>>> I say "may" ;) >>>>> You don't need new function if there is no use. >>>>> >>>>> > >>>>> > So to summarize, the current consensus: >>>>> > - end can be renamed either to currentend or fastpathend; that is >>>>> > still up to a vote and choice :) >>>>> >>>>> Please, use currentend. I was thinking about sampleend but it does not reflect double usage. >>>> >>>> I'm not sure if you have seen the discussion about naming on hotspot-gc-dev, but I and others think that >>>> currentend doesn't describe the usage at all. Naming it fastpathend would clearly state what it is, sampleend >>>> or something similar also crossed my mind but I think the code reads a lot better in the compiler with the name >>>> fastpathend. >>>> >>>>> >>>>> > - the access method end() and tlabendoffset() remain the same to not >>>>> > modify JIT/interpreter codes >>>> I would find this very unfortunate, having accessors that don't match the members can easily lead to >>>> misunderstanding, especially if we have three different *end members. Why do you think this is the best way forward? >>>> >>>> My first thought was also that it would be nice to avoid all the compiler changes, but then we would have to stick >>>> with end being the sample/current/fast-path end and I'm not sure that is a better solution. I don't see the big >>>> problem with changing those accessors to what they really access and I think the compiler code reads good even after >>>> the change. For example: >>>> cmpptr(end, Address(thread, JavaThread::tlabfastpathendoffset())); >>>> jcc(Assembler::above, slowcase); >>>> >>>>> > >>>>> > If all agree to this, then the consequences are: >>>>> > - JDK-8201326 becomes useless >>>>> > - The work for JEP-331 becomes smaller in terms of file changes >>>>> > - I'll still be needing a decision on the renaming of the TLAB end field >>>>> > (current suggestions are currentend and fastpathend). >>>> >>>> We'll see where we end up. If JDK-8201326 ends up being a separate change I think it should be pushed at the same >>>> time as the rest of the JEP changes. To me it only makes sense to have it in the code base if we also have the rest >>>> of the changes. >>>> >>>> Thanks, >>>> Stefan >>>> >>>>> >>>>> Sounds good to me. >>>>> >>>>> Thanks, >>>>> Vladimir >>>>> >>>>> On 4/17/18 4:51 PM, JC Beyler wrote: >>>>>> Hi Vladimir and Dean, >>>>>> >>>>>> @Dean: seems that Vladimir is in agreement with you for renaming just the >>>>>> field and not the method names so ack to your answer that came at the same >>>>>> time :) >>>>>> >>>>>> >>>>>>> Yes, from the beginning such changes should be discussed on common >>>>>>> hotspot-dev list since all >>>>>>> Hotspot's parts are affected. >>>>>>> >>>>>> >>>>>> Sorry, being new to the scene, most of the conversation had been going on >>>>>> in serviceability-dev. >>>>>> >>>>>> >>>>>>> >>>>>>> Graal specific question could be send to hotspot-compiler-dev list with >>>>>>> [Graal] in subject. >>>>>>> >>>>>>> I looked on JEP's changes >>>>>>> http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02/ to understand how >>>>>>> it works. >>>>>>> >>>>>>> Few questions about proposed JEP changes so I can understand it. >>>>>>> >>>>>>> You introducing new TLAB end for sampling and adjust it so that >>>>>>> allocations in JITed code and >>>>>>> Interpreter it will trigger slow path allocation where you do sampling. >>>>>>> Right? >>>>>>> >>>>>> >>>>>> Yes that is correct; if sampling is enabled of course. Btw, this is the current >>>>>> webrev <http://cr.openjdk.java.net/~jcbeyler/8171119/heapevent.15/>. >>>>>> >>>>>> >>>>>>> >>>>>>> Do all changes to end, actualend and other TLAB fields happen during >>>>>>> slow allocation? >>>>>>> >>>>>> >>>>>> Yes, to that effect, with Robbin's help, we finalized deprecating the >>>>>> FastTLabRefill via JDK-8194084 >>>>>> <https://bugs.openjdk.java.net/browse/JDK-8194084>. Seems like I/we missed >>>>>> that Graal does this as well. I filed GRAAL-64 >>>>>> <https://bugs.openjdk.java.net/browse/GRAAL-64> to not forget that Graal >>>>>> would need to get that fixed. >>>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> I am concern about concurrent accesses to these fields from other threads >>>>>>> if you have them (I don't >>>>>>> see). >>>>>>> >>>>>> >>>>>> Yes that is why we deprecated the FastTlabRefill. Other threads should not >>>>>> be changing the thread local data structure so I don't see an issue there. >>>>>> The major issue was that the fast paths could change the tlab without going >>>>>> via the slowpath. >>>>>> >>>>>> I had a fix to detect this issue but removed it once JDK-8194084 was done; >>>>>> Graal was missed in that work so that is why if sampling were enabled with >>>>>> Graal, there is a chance things would break currently. That will get fixed >>>>>> via GRAAL-64 <https://bugs.openjdk.java.net/browse/GRAAL-64> whether it is >>>>>> rendering the code also obsolete and going to the slowpath or whether it is >>>>>> adding my fix again to detect a fastpath TLAB reallocation happened. >>>>>> >>>>>> >>>>>>> >>>>>>> Renaming. I am fine with renaming if it helps to understand code better. I >>>>>>> agree with proposed >>>>>>> changes to fields name: >>>>>>> >>>>>>> currentend >>>>>>> allocationend >>>>>>> >>>>>>> But, as Dean, I would suggest to keep names of access functions. This way >>>>>>> we can say that allocation >>>>>>> code in Interpreter and JITed code stay the same. >>>>>>> >>>>>> >>>>>> Sounds good to me, then in that case, this webrev will disappear, which >>>>>> also makes me happy, since it simplifies a lot of things (and reduces code >>>>>> change). >>>>>> >>>>>> >>>>>>> >>>>>>> You may need only new method to access allocationend in places which >>>>>>> look for real end of TLAB. >>>>>>> >>>>>> >>>>>> I think I like better not to add it. If no one is using it, there should be >>>>>> no reason to add it? By not adding it, it also makes any future wish to >>>>>> have it a nice indicator of: hey why do you want to see this? Same as >>>>>> hardend btw which is not visible. Am I missing something? >>>>>> >>>>>> So to summarize, the current consensus: >>>>>> - end can be renamed either to currentend or fastpathend; that is >>>>>> still up to a vote and choice :) >>>>>> - the access method end() and tlabendoffset() remain the same to not >>>>>> modify JIT/interpreter codes >>>>>> >>>>>> If all agree to this, then the consequences are: >>>>>> - JDK-8201326 becomes useless >>>>>> - The work for JEP-331 becomes smaller in terms of file changes >>>>>> - I'll still be needing a decision on the renaming of the TLAB end field >>>>>> (current suggestions are currentend and fastpathend). >>>>>> >>>>>> Thanks for your help! >>>>>> Jc >>>>>> >>>>>> >>>>>>> >>>>>>> Regards, >>>>>>> Vladimir >>>>>>> >>>>>>> On 4/16/18 4:42 PM, JC Beyler wrote: >>>>>>>> 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 >>>>>>>> allocationend: the actual allocation end of the current TLAB >>>>>>>> hardend: allocationend + 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: >>>>>>>> >>>>>>>> currentend: the fast path end >>>>>>>> allocationend: the actual allocation end of the current TLAB >>>>>>>> reservedend: allocationend + 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: >>>>>>>> >>>>>>>> fastpathend: the fast path end >>>>>>>> allocationend: the actual allocation end of the current TLAB >>>>>>>> hardend: allocationend + 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 fastpathend 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: >>>>>>>> - Should we rename the end field and thus provoke the changes in >>>>>>> this >>>>>>>> webrev? >>>>>>>> - If we do want to change the field, should/could it go in an initial >>>>>>>> webrev or should it go in the JEP-331 webrev whenever/ifever it goes in? >>>>>>>> And if we do wait, could we at least converge on a renaming now so that >>>>>>>> this does not become a point of contention for that webrev's review? >>>>>>>> >>>>>>>> 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 <mailto: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 <mailto: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> <mailto: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>> >>>>>>>>>>>> <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, >>>>>>>>>>>> > >>>>>>>>>>>> > 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> >>>>>>>>> >>>>>>>>>>>> > > <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 <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>>> >>>>>>>>>>>> > <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 <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>>> >>>>>>>>>>>> > >> <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 <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> >>>>>>>>>>>> > <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? >>>>>>>>>>>> > > >>>>>>>>>>>> > >>>>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>> >>> >
- Previous message: RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
- Next message: RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]