<div dir="ltr">Hi Stefan,<div><br></div><div>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?</div><div><br></div><div>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?</div><div><br></div><div>Thanks for all your help,</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Thu, Apr 12, 2018 at 2:09 AM Stefan Johansson <<a href="mailto:stefan.johansson@oracle.com">stefan.johansson@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
On 2018-04-11 17:48, JC Beyler wrote:<br>
> Hi Stefan,<br>
> <br>
> Sorry about that, I must have missed adding the files or something. Here <br>
> is a webrev that added the changes for the SA file:<br>
> <a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.03/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.03/</a><br>
> <br>
No problem, this looks good. I kicked of a run in our test system to get <br>
some more coverage and have tried to include some Graal testing. I'll <br>
let you know the results once they are done.<br>
<br>
Please also update the bug title both in JBS and the changeset.<br>
<br>
Cheers,<br>
Stefan<br>
<br>
> I changed the method name, which propagated a change to:<br>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ObjectHeap.java<br>
> <br>
> I tried testing your test file. It exists in my branch (if the same) under:<br>
> hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java<br>
> <br>
> and interestingly (which generally means I did something wrong), it <br>
> passed before/after the change so I could not verify that this is a test <br>
> showing that all is well in the world on my side. Any ideas of what I <br>
> did wrong?<br>
> <br>
> I did again test it for hotspot/jtreg/compiler/aot/ and <br>
> hotspot/jtreg/serviceability/jvmti and it passes those.<br>
> <br>
> Thanks for all your help,<br>
> Jc<br>
> <br>
> <br>
> <br>
> On Wed, Apr 11, 2018 at 7:44 AM Stefan Johansson <br>
> <<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a> <mailto:<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a>>> wrote:<br>
> <br>
> Hi JC,<br>
> <br>
> On 2018-04-11 00:56, JC Beyler wrote:<br>
> > Small update:<br>
> ><br>
> > Here is the fixed webrev for the '{' that were out of alignment.<br>
> This<br>
> > passed release build JTREG for hotspot/jtreg/compiler/jvmti (just<br>
> to run<br>
> > something as a smoke screen) and hotspot/jtreg/compiler/aot/ (to<br>
> test<br>
> > Graal).<br>
> > <a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.02/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.02/</a><br>
> ><br>
> I think this looks better, I agree that leaving _end is tempting to<br>
> avoid a lot of change, but I think this will be better in the long run.<br>
> <br>
> I still miss the changes to make the SA work. To see a problem you<br>
> can run:<br>
> make CONF=fast run-test<br>
> TEST=open/test/hotspot/jtreg/serviceability/ClhsdbJhisto.java<br>
> <br>
> Cheers,<br>
> Stefan<br>
> <br>
> > Let me know what you think,<br>
> > Jc<br>
> ><br>
> > On Tue, Apr 10, 2018 at 3:21 PM JC Beyler <<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a><br>
> <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>><br>
> > <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a> <mailto:<a href="mailto:jcbeyler@google.com" target="_blank">jcbeyler@google.com</a>>>> wrote:<br>
> ><br>
> > Hi Karen and Stefan,<br>
> ><br>
> > @Stefan: Naming is hard :)<br>
> > @Karen: thanks for the Graal comment, I fixed it in the new<br>
> webrev,<br>
> > let me know what you think :)<br>
> ><br>
> > I think the naming convention suggested in this webrev came from<br>
> > conversations in for JEP-331 and I am actually relatively<br>
> > indifferent to the final result (as long as we have some form of<br>
> > forward progress :)). To be honest, I'd also be happy to just<br>
> leave<br>
> > _end as is for all architectures and Graal and have a new<br>
> > _allocation_end. However, during initial reviews of JEP-331<br>
> it was<br>
> > deemed complicated to understand:<br>
> ><br>
> > _end -> the _end or sampling end<br>
> > _allocation_end -> end pointer for the last possible allocation<br>
> > hard_end -> allocation end + reserved space<br>
> ><br>
> > That is how this naming came up and why hard_end went to<br>
> "reserved_end".<br>
> ><br>
> > I'm really indifferent, so I offer as a perusal:<br>
> > <a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.01/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.01/</a><br>
> ><br>
> > I noticed a few problems of alignement of '{' so I'll go fix<br>
> that.<br>
> > Basically this webrev does the following:<br>
> ><br>
> > - Uses fast_path_end instead of end<br>
> > - Reverts hard_end back to where it was<br>
> > - Adds the changes to Graal; there is a bunch of changes in Graal<br>
> > because Graal still contains a bit of code doing fasttlabrefills.<br>
> ><br>
> > Let me know what you think!<br>
> ><br>
> > Thanks,<br>
> > Jc<br>
> ><br>
> ><br>
> > On Tue, Apr 10, 2018 at 6:56 AM Karen Kinnear<br>
> > <<a href="mailto:karen.kinnear@oracle.com" target="_blank">karen.kinnear@oracle.com</a> <mailto:<a href="mailto:karen.kinnear@oracle.com" target="_blank">karen.kinnear@oracle.com</a>><br>
> <mailto:<a href="mailto:karen.kinnear@oracle.com" target="_blank">karen.kinnear@oracle.com</a> <mailto:<a href="mailto:karen.kinnear@oracle.com" target="_blank">karen.kinnear@oracle.com</a>>>><br>
> wrote:<br>
> ><br>
> > Hi JC,<br>
> ><br>
> > A comment about Graal. The impact on Graal for this<br>
> particular<br>
> > change would be to break it - so you’ll need<br>
> > to complete the Graal changes for this renaming. That isn’t<br>
> > optional or something that could be a follow-on. It<br>
> > is not ok to break a feature, even an experimental one.<br>
> We will<br>
> > discuss in the other thread potential phasing of adding<br>
> sampling.<br>
> ><br>
> > I did not do a thorough search -you can do that - I did find<br>
> > src/jdk.internal.vm.compiler/share/classes/<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > public final int threadTlabOffset =<br>
> > getFieldOffset("Thread::_tlab", Integer.class,<br>
> > "ThreadLocalAllocBuffer");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int threadLocalAllocBufferStartOffset =<br>
> > getFieldOffset("ThreadLocalAllocBuffer::_start",<br>
> Integer.class,<br>
> > "HeapWord*");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int threadLocalAllocBufferEndOffset =<br>
> > getFieldOffset("ThreadLocalAllocBuffer::_end", Integer.class,<br>
> > "HeapWord*");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int threadLocalAllocBufferTopOffset =<br>
> > getFieldOffset("ThreadLocalAllocBuffer::_top", Integer.class,<br>
> > "HeapWord*");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int threadLocalAllocBufferPfTopOffset =<br>
> > getFieldOffset("ThreadLocalAllocBuffer::_pf_top",<br>
> Integer.class,<br>
> > "HeapWord*");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int<br>
> threadLocalAllocBufferSlowAllocationsOffset<br>
> > = getFieldOffset("ThreadLocalAllocBuffer::_slow_allocations",<br>
> > Integer.class, "unsigned");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int<br>
> threadLocalAllocBufferFastRefillWasteOffset<br>
> > =<br>
> getFieldOffset("ThreadLocalAllocBuffer::_fast_refill_waste",<br>
> > Integer.class, "unsigned");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int<br>
> threadLocalAllocBufferNumberOfRefillsOffset<br>
> > =<br>
> getFieldOffset("ThreadLocalAllocBuffer::_number_of_refills",<br>
> > Integer.class, "unsigned");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int<br>
> > threadLocalAllocBufferRefillWasteLimitOffset =<br>
> > getFieldOffset("ThreadLocalAllocBuffer::_refill_waste_limit",<br>
> > Integer.class, "size_t");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > private final int<br>
> threadLocalAllocBufferDesiredSizeOffset =<br>
> > getFieldOffset("ThreadLocalAllocBuffer::_desired_size",<br>
> > Integer.class, "size_t");<br>
> > <br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:<br>
> > public final int tlabAlignmentReserve =<br>
> > <br>
> getFieldValue("CompilerToVM::Data::ThreadLocalAllocBuffer_alignment_reserve",<br>
> > Integer.class, "size_t”);<br>
> ><br>
> > hope this helps,<br>
> > Karen<br>
> ><br>
> >> On Apr 10, 2018, at 7:04 AM, Stefan Johansson<br>
> >> <<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a><br>
> <mailto:<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a>><br>
> >> <mailto:<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a><br>
> <mailto:<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a>>>> wrote:<br>
> >><br>
> >> Hi JC,<br>
> >><br>
> >> I realize that the names have been discussed before but I'm<br>
> >> leaning towards suggesting something new. We discussed this<br>
> >> briefly here in the office and others might have different<br>
> >> opinions. One point that came up is that if we do this<br>
> change<br>
> >> and change all usages of JavaThread::tlab_end_offset() it<br>
> >> would be good to make sure the new name is good. To us<br>
> >> _current_end is not very descriptive, but naming is hard and<br>
> >> the best we could come up with is _fast_path_end which would<br>
> >> give the code:<br>
> >> cmpptr(end, Address(thread,<br>
> >> JavaThread::tlab_fast_path_end_offset()));<br>
> >> jcc(Assembler::above, slow_case);<br>
> >><br>
> >> I think this reads pretty good and is fairly clear. If we do<br>
> >> this rename I think you can re-use _end in JEP-331<br>
> instead of<br>
> >> calling it _allocation_end. But that is a later review :)<br>
> >><br>
> >> Also, is there a good reason for renaming hard_end() to<br>
> >> reserved_end()?<br>
> >><br>
> >> One additional thing, you need to update the SA to reflect<br>
> >> this change. I think the only place you need to fix is in:<br>
> >> <br>
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/ThreadLocalAllocBuffer.java<br>
> >><br>
> >> Thanks,<br>
> >> Stefan<br>
> >><br>
> >> On 2018-04-09 19:24, JC Beyler wrote:<br>
> >>> Hi all,<br>
> >>> Small pre-amble to this request:<br>
> >>> In my work to try to get a heap sampler in OpenJDK (via JEP<br>
> >>> 331<br>
> <<a href="https://bugs.openjdk.java.net/browse/JDK-8171119" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8171119</a>>), I'm<br>
> >>> trying to reduce the footprint of my change so that the<br>
> >>> integration can be easier. I was told that generally a JEP<br>
> >>> webrev should be feature complete and go in at-once.<br>
> However,<br>
> >>> with the change touching quite a bit of various code<br>
> pieces,<br>
> >>> I was trying to figure out what could be separated as not<br>
> >>> "part of the feature".<br>
> >>> I asked around and said that perhaps a solution would be to<br>
> >>> cut up the renaming of TLAB's end field that I do in that<br>
> >>> webrev. Because I'm renaming a field in TLAB used by<br>
> various<br>
> >>> backends for that work, I have to update every architecture<br>
> >>> dependent code to reflect it.<br>
> >>> I entirely understand that perhaps this is not in the<br>
> habits<br>
> >>> and very potentially might not be the way things are<br>
> >>> generally done. If so, I apologize and let me know if you<br>
> >>> would not want this to go in separately :)<br>
> >>> Final note: there is still a chance JEP-331 does not go in.<br>
> >>> If it does not, we can leave the new name in place or I'll<br>
> >>> happily revert it. I can even create an issue to track this<br>
> >>> if that makes it easier for all.<br>
> >>> End of the pre-amble.<br>
> >>> The 33-line change webrev in question is here:<br>
> >>> <a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.00/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.00/</a><br>
> >>> I fixed all the architectures and JVMCI and ran a few<br>
> sanity<br>
> >>> tests to ensure I had not missed anything.<br>
> >>> Thanks for your help and I hope this is not too much<br>
> trouble,<br>
> >>> Jc<br>
> >>> Ps: there is a graal change that needs to happen but I was<br>
> >>> not sure <a href="https://teams.googleplex.com/u/where" target="_blank">who/where</a><br>
> <<a href="https://teams.googleplex.com/u/where" rel="noreferrer" target="_blank">https://teams.googleplex.com/u/where</a>><br>
> <<a href="https://teams.googleplex.com/u/where" rel="noreferrer" target="_blank">https://teams.googleplex.com/u/where</a>> to<br>
> >>> ask about it. I was told it could happen in a separate<br>
> >>> webrev. Can anyone point me to the right direction?<br>
> Should it<br>
> >>> just be hotspot-compiler-dev?<br>
> ><br>
> <br>
</blockquote></div>