RFR (S): 8201326: Renaming ThreadLocalAllocationBuffer end to current_end
Stefan Johansson
stefan.johansson at oracle.com
Thu Apr 12 15:20:02 UTC 2018
Hi again,
On 2018-04-12 11:09, Stefan Johansson 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.
>
The normal testing looks good, but the Graal task I started had some
errors. To me they didn't look related to your change but you should
probably reach out to a compiler engineer and ask them to run through
some Graal testing to be sure.
Cheers,
Stefan
> 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>> 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>>> 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>>>
>> 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>>> 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> 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-gc-dev
mailing list