RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
Stefan Johansson
stefan.johansson at oracle.com
Wed Apr 18 08:04:11 UTC 2018
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