RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
dean.long at oracle.com
dean.long at oracle.com
Wed Apr 18 19:15:52 UTC 2018
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(); }
even though end_offset() would then refer to a virtual "end" field.
Vladimir, are you also approving the Graal changes? :-)
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> 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