RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
Daniel D. Daugherty
daniel.daugherty at oracle.com
Wed Apr 18 13:29:08 UTC 2018
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