RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Apr 18 23:02:56 UTC 2018
On 4/18/18 15:55, serguei.spitsyn at oracle.com wrote:
> Vladimir,
>
> Thanks a lot for making this resolved!
> It safes our time as we will stop all these renaming discussions now.
A typo above: safes => saves
Sorry for spamming.
Thanks,
Serguei
> On 4/18/18 15:06, Vladimir Kozlov wrote:
>> 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.
>
> +1
> One more question is if we have to close this enhancement as NAI or to
> keep it for post JEP renaming.
> I'd prefer to close it.
>
>>
>> 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
>
> Right.
> It looks like the only way to move forward.
> It will make it simpler.
>
> Thanks,
> Serguei
>
>>>
>>> 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