RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to fast_path_end
Vladimir Kozlov
vladimir.kozlov at oracle.com
Wed Apr 18 19:46:33 UTC 2018
On 4/18/18 12:15 PM, dean.long at oracle.com wrote:
> I will defer to Vladimir, but I would have been happy with something like:
>
> // preserve simple start/end abstraction for compiler
> HeapWord* end() const { return fast_path_end(); }
> static ByteSize end_offset() { return fast_path_end_offset(); }
This is ugly.
>
> even though end_offset() would then refer to a virtual "end" field.
>
> Vladimir, are you also approving the Graal changes? :-)
You really made my day :(
S..t! We can't make this change in Graal as suggested because we will break Graal's JDK 8 support.
Graal has direct access to VM's fields through JVMCI. You have to guard change with JDK version check.
Labs start addressing multi-releases so it may be not big issue anymore. See Doug's comment for 8201318 RFR.
Anyway you have to file new PR (pull request) for Graal changes on https://github.com/graalvm/mx/pulls.
And I am not sponsoring this. I don't think such renaming worse all our efforts.
Good luck,
Vladimir
>
> dl
>
> On 4/18/18 11:02 AM, Vladimir Kozlov wrote:
>> Ganging up on us ;)
>>
>> Yes, I missed original discussion about renaming on GC list.
>>
>> From my point of view next code looks better because it seems compare related values:
>>
>> cmpptr(end, Address(thread, JavaThread::tlab_end_offset()));
>>
>> But I would not strongly object renaming to move this JEP forward. Consider changes reviewed and approved by me.
>>
>> Regards,
>> Vladimir
>>
>> On 4/18/18 6:29 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> The idea of splitting this change off from "8171119: Low-Overhead Heap Profiling"
>>> came up during the design review. It might have been me that suggested the split
>>> off or it was someone else. Sorry I don't remember.
>>>
>>> In any case, the rename of "end" to "fast_path_end" is just a clarity change to
>>> the existing code and I think that change can easily stand on its own. That is
>>> particularly true if the accessors are renamed at the same time. I think having
>>> the accessor names match the field names is a pretty well known HotSpot rule
>>> so I'm not sure why we're talking about not renaming the accessors...
>>>
>>> Personally, I prefer "_fast_path_end" over "_current_end".
>>>
>>> Dan
>>>
>>>
>>> On 4/18/18 4:04 AM, Stefan Johansson wrote:
>>>> Hi,
>>>>
>>>> On 2018-04-18 02:02, Vladimir Kozlov wrote:
>>>>> > I think I like better not to add it. If no one is using it, there should be
>>>>> > no reason to add it? By not adding it, it also makes any future wish to
>>>>> > have it a nice indicator of: hey why do you want to see this? Same as
>>>>> > hard_end btw which is not visible. Am I missing something?
>>>>>
>>>>> I say "may" ;)
>>>>> You don't need new function if there is no use.
>>>>>
>>>>> >
>>>>> > So to summarize, the current consensus:
>>>>> > - _end can be renamed either to _current_end or _fast_path_end; that is
>>>>> > still up to a vote and choice :)
>>>>>
>>>>> Please, use _current_end. I was thinking about _sample_end but it does not reflect double usage.
>>>>
>>>> I'm not sure if you have seen the discussion about naming on hotspot-gc-dev, but I and others think that
>>>> _current_end doesn't describe the usage at all. Naming it _fast_path_end would clearly state what it is, _sample_end
>>>> or something similar also crossed my mind but I think the code reads a lot better in the compiler with the name
>>>> fast_path_end.
>>>>
>>>>>
>>>>> > - the access method end() and tlab_end_offset() remain the same to not
>>>>> > modify JIT/interpreter codes
>>>> I would find this very unfortunate, having accessors that don't match the members can easily lead to
>>>> misunderstanding, especially if we have three different *_end members. Why do you think this is the best way forward?
>>>>
>>>> My first thought was also that it would be nice to avoid all the compiler changes, but then we would have to stick
>>>> with _end being the sample/current/fast-path end and I'm not sure that is a better solution. I don't see the big
>>>> problem with changing those accessors to what they really access and I think the compiler code reads good even after
>>>> the change. For example:
>>>> cmpptr(end, Address(thread, JavaThread::tlab_fast_path_end_offset()));
>>>> jcc(Assembler::above, slow_case);
>>>>
>>>>> >
>>>>> > If all agree to this, then the consequences are:
>>>>> > - JDK-8201326 becomes useless
>>>>> > - The work for JEP-331 becomes smaller in terms of file changes
>>>>> > - I'll still be needing a decision on the renaming of the TLAB _end field
>>>>> > (current suggestions are _current_end and _fast_path_end).
>>>>
>>>> We'll see where we end up. If JDK-8201326 ends up being a separate change I think it should be pushed at the same
>>>> time as the rest of the JEP changes. To me it only makes sense to have it in the code base if we also have the rest
>>>> of the changes.
>>>>
>>>> Thanks,
>>>> Stefan
>>>>
>>>>>
>>>>> Sounds good to me.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 4/17/18 4:51 PM, JC Beyler wrote:
>>>>>> Hi Vladimir and Dean,
>>>>>>
>>>>>> @Dean: seems that Vladimir is in agreement with you for renaming just the
>>>>>> field and not the method names so ack to your answer that came at the same
>>>>>> time :)
>>>>>>
>>>>>>
>>>>>>> Yes, from the beginning such changes should be discussed on common
>>>>>>> hotspot-dev list since all
>>>>>>> Hotspot's parts are affected.
>>>>>>>
>>>>>>
>>>>>> Sorry, being new to the scene, most of the conversation had been going on
>>>>>> in serviceability-dev.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Graal specific question could be send to hotspot-compiler-dev list with
>>>>>>> [Graal] in subject.
>>>>>>>
>>>>>>> I looked on JEP's changes
>>>>>>> http://cr.openjdk.java.net/~jcbeyler/8171119/webrev.02/ to understand how
>>>>>>> it works.
>>>>>>>
>>>>>>> Few questions about proposed JEP changes so I can understand it.
>>>>>>>
>>>>>>> You introducing new TLAB end for sampling and adjust it so that
>>>>>>> allocations in JITed code and
>>>>>>> Interpreter it will trigger slow path allocation where you do sampling.
>>>>>>> Right?
>>>>>>>
>>>>>>
>>>>>> Yes that is correct; if sampling is enabled of course. Btw, this is the current
>>>>>> webrev <http://cr.openjdk.java.net/~jcbeyler/8171119/heap_event.15/>.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Do all changes to _end, _actual_end and other TLAB fields happen during
>>>>>>> slow allocation?
>>>>>>>
>>>>>>
>>>>>> Yes, to that effect, with Robbin's help, we finalized deprecating the
>>>>>> FastTLabRefill via JDK-8194084
>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8194084>. Seems like I/we missed
>>>>>> that Graal does this as well. I filed GRAAL-64
>>>>>> <https://bugs.openjdk.java.net/browse/GRAAL-64> to not forget that Graal
>>>>>> would need to get that fixed.
>>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> I am concern about concurrent accesses to these fields from other threads
>>>>>>> if you have them (I don't
>>>>>>> see).
>>>>>>>
>>>>>>
>>>>>> Yes that is why we deprecated the FastTlabRefill. Other threads should not
>>>>>> be changing the thread local data structure so I don't see an issue there.
>>>>>> The major issue was that the fast paths could change the tlab without going
>>>>>> via the slowpath.
>>>>>>
>>>>>> I had a fix to detect this issue but removed it once JDK-8194084 was done;
>>>>>> Graal was missed in that work so that is why if sampling were enabled with
>>>>>> Graal, there is a chance things would break currently. That will get fixed
>>>>>> via GRAAL-64 <https://bugs.openjdk.java.net/browse/GRAAL-64> whether it is
>>>>>> rendering the code also obsolete and going to the slowpath or whether it is
>>>>>> adding my fix again to detect a fastpath TLAB reallocation happened.
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Renaming. I am fine with renaming if it helps to understand code better. I
>>>>>>> agree with proposed
>>>>>>> changes to fields name:
>>>>>>>
>>>>>>> _current_end
>>>>>>> _allocation_end
>>>>>>>
>>>>>>> But, as Dean, I would suggest to keep names of access functions. This way
>>>>>>> we can say that allocation
>>>>>>> code in Interpreter and JITed code stay the same.
>>>>>>>
>>>>>>
>>>>>> Sounds good to me, then in that case, this webrev will disappear, which
>>>>>> also makes me happy, since it simplifies a lot of things (and reduces code
>>>>>> change).
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> You may need only new method to access _allocation_end in places which
>>>>>>> look for real end of TLAB.
>>>>>>>
>>>>>>
>>>>>> I think I like better not to add it. If no one is using it, there should be
>>>>>> no reason to add it? By not adding it, it also makes any future wish to
>>>>>> have it a nice indicator of: hey why do you want to see this? Same as
>>>>>> hard_end btw which is not visible. Am I missing something?
>>>>>>
>>>>>> So to summarize, the current consensus:
>>>>>> - _end can be renamed either to _current_end or _fast_path_end; that is
>>>>>> still up to a vote and choice :)
>>>>>> - the access method end() and tlab_end_offset() remain the same to not
>>>>>> modify JIT/interpreter codes
>>>>>>
>>>>>> If all agree to this, then the consequences are:
>>>>>> - JDK-8201326 becomes useless
>>>>>> - The work for JEP-331 becomes smaller in terms of file changes
>>>>>> - I'll still be needing a decision on the renaming of the TLAB _end field
>>>>>> (current suggestions are _current_end and _fast_path_end).
>>>>>>
>>>>>> Thanks for your help!
>>>>>> Jc
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Vladimir
>>>>>>>
>>>>>>> On 4/16/18 4:42 PM, JC Beyler wrote:
>>>>>>>> Hi Dean,
>>>>>>>>
>>>>>>>> I think perhaps this is also an attempt to figure out the naming of all
>>>>>>>> this because naming (or renaming like here) is often the start of big
>>>>>>>> conversations :).
>>>>>>>>
>>>>>>>> Originally, in the JEP-331 work, I had left the _end field as is and, by
>>>>>>>> doing so, this whole renaming webrev goes away. However, if we do that,
>>>>>>>> then the TLAB code contains:
>>>>>>>>
>>>>>>>> _end: the fast path end, can be the allocation end or the to-be-sampled
>>>>>>> end
>>>>>>>> _allocation_end: the actual allocation end of the current TLAB
>>>>>>>> hard_end: _allocation_end + reserved space
>>>>>>>>
>>>>>>>> With an early iteration of a webrev for JEP-331, a conversation occurred
>>>>>>>> about whether or not that was clear or not and it was determined that
>>>>>>> this
>>>>>>>> renaming was more clear:
>>>>>>>>
>>>>>>>> _current_end: the fast path end
>>>>>>>> _allocation_end: the actual allocation end of the current TLAB
>>>>>>>> reserved_end: _allocation_end + reserved space
>>>>>>>>
>>>>>>>> Because I'm trying to reduce the footprint of files changed, I pulled out
>>>>>>>> the renaming changes into this webrev. While talking about it with the GC
>>>>>>>> team, the renaming suggested became:
>>>>>>>>
>>>>>>>> _fast_path_end: the fast path end
>>>>>>>> _allocation_end: the actual allocation end of the current TLAB
>>>>>>>> hard_end: _allocation_end + reserved space
>>>>>>>>
>>>>>>>> Now, to be honest, any renaming or no renaming is fine by me. I like not
>>>>>>>> renaming the fields to not change the code of every backend and Graal; I
>>>>>>>> also like the fast_path_end rename due to it making the backend code easy
>>>>>>>> to read as mentioned in the GC mailing lis thread.
>>>>>>>>
>>>>>>>> So there are two questions really:
>>>>>>>> - Should we rename the _end field and thus provoke the changes in
>>>>>>> this
>>>>>>>> webrev?
>>>>>>>> - If we do want to change the field, should/could it go in an initial
>>>>>>>> webrev or should it go in the JEP-331 webrev whenever/ifever it goes in?
>>>>>>>> And if we do wait, could we at least converge on a renaming now so that
>>>>>>>> this does not become a point of contention for that webrev's review?
>>>>>>>>
>>>>>>>> If I read your answer correctly:
>>>>>>>> - You are saying that we should keep the _end field altogether; or
>>>>>>> at
>>>>>>>> least only rename the field not the methods to access it, thus reducing
>>>>>>> the
>>>>>>>> change scope.
>>>>>>>> - You are also saying to wait for the JEP-331 webrev's final
>>>>>>> iteration
>>>>>>>> and integrate it in one step
>>>>>>>>
>>>>>>>> Have I understood your answer correctly?
>>>>>>>>
>>>>>>>> Again, I am fine with renaming to whatever or not renaming at all. I just
>>>>>>>> am trying to get forward progress here :)
>>>>>>>>
>>>>>>>> Thanks for your help!
>>>>>>>> Jc
>>>>>>>>
>>>>>>>> On Mon, Apr 16, 2018 at 3:29 PM <dean.long at oracle.com> 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