RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
JC Beyler
jcbeyler at google.com
Wed Apr 18 21:54:42 UTC 2018
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 Heap Profiling" 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).
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>
wrote:
> On 4/18/18 12:15 PM, 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> 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> 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
> >>>>>>>>>>>> > > _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>>>>
> >>>>>>>>>>>> > 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>>>> 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