RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Apr 18 22:06:42 UTC 2018
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
> _allocation_end: the actual allocation end of the current TLAB
> hard_end: _allocation_end + 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 fast_path_end(); }
> > static ByteSize end_offset() { return fast_path_end_offset(); }
>
> This is ugly.
>
> >
> > even though end_offset() 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::tlab_end_offset()));
> >>
> >> 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 "fast_path_end" 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 "_fast_path_end" over "_current_end".
> >>>
> >>> 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
> >>>>> > hard_end 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 _current_end or _fast_path_end; that is
> >>>>> > still up to a vote and choice :)
> >>>>>
> >>>>> Please, use _current_end. I was thinking about _sample_end 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
> >>>> _current_end doesn't describe the usage at all. Naming it _fast_path_end would clearly
> state what it is, _sample_end
> >>>> or something similar also crossed my mind but I think the code reads a lot better in the
> compiler with the name
> >>>> fast_path_end.
> >>>>
> >>>>>
> >>>>> > - the access method end() and tlab_end_offset() 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::tlab_fast_path_end_offset()));
> >>>> jcc(Assembler::above, slow_case);
> >>>>
> >>>>> >
> >>>>> > 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 _current_end and _fast_path_end).
> >>>>
> >>>> 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/heap_event.15/>.
> >>>>>>
> >>>>>>
> >>>>>>>
> >>>>>>> Do all changes to _end, _actual_end 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:
> >>>>>>>
> >>>>>>> _current_end
> >>>>>>> _allocation_end
> >>>>>>>
> >>>>>>> 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 _allocation_end 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
> >>>>>> hard_end btw which is not visible. Am I missing something?
> >>>>>>
> >>>>>> So to summarize, the current consensus:
> >>>>>> - _end can be renamed either to _current_end or _fast_path_end; that is
> >>>>>> still up to a vote and choice :)
> >>>>>> - the access method end() and tlab_end_offset() 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 _current_end and _fast_path_end).
> >>>>>>
> >>>>>> 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
> >>>>>>>> _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:
> >>>>>>>> - 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 fast_path_end(), then isn't it logical
> >>>>>>>>> to have fast_path_start() paired with it? ("start" is already
> >>>>>>>>> overloaded, but nobody is suggesting adding
> >>>>>>>>> allocation_start()/current_start()/hard_start() 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
> >>>>>>>>>>>> > > _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>>>
> >>>>>>>>>>>> > <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::_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>>>
> >>>>>>>>>>>> > >> <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::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>
> >>>>>>>>>>> <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