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