RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Apr 18 18:02:12 UTC 2018
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