<div dir="ltr">Hi Stefan,<div><br></div><div>Sorry about that, I must have missed adding the files or something. Here is a webrev that added the changes for the SA file:</div><div><a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.03/">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.03/</a><br></div><div><br></div><div>I changed the method name, which propagated a change to:</div><div>src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/oops/ObjectHeap.java</div><div><br></div><div>I tried testing your test file. It exists in my branch (if the same) under:</div><div>hotspot/jtreg/serviceability/sa/ClhsdbJhisto.java<br></div><div><br></div><div>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? </div><div><br></div><div>I did again test it for hotspot/jtreg/compiler/aot/ and hotspot/jtreg/serviceability/jvmti and it passes those.</div><div><br></div><div>Thanks for all your help,</div><div>Jc</div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Wed, Apr 11, 2018 at 7:44 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">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. This <br>
> passed release build JTREG for hotspot/jtreg/compiler/jvmti (just to run <br>
> something as a smoke screen) and hotspot/jtreg/compiler/aot/ (to 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 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>>> 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 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 leave<br>
> _end as is for all architectures and Graal and have a new<br>
> _allocation_end. However, during initial reviews of JEP-331 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 "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 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>>> wrote:<br>
> <br>
> Hi JC,<br>
> <br>
> A comment about Graal. The impact on Graal for this 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. We will<br>
> discuss in the other thread potential phasing of adding 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>
> 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>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> private final int threadLocalAllocBufferStartOffset =<br>
> getFieldOffset("ThreadLocalAllocBuffer::_start", Integer.class,<br>
> "HeapWord*");<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>
> 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>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> private final int threadLocalAllocBufferPfTopOffset =<br>
> getFieldOffset("ThreadLocalAllocBuffer::_pf_top", Integer.class,<br>
> "HeapWord*");<br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> private final int threadLocalAllocBufferSlowAllocationsOffset<br>
> = getFieldOffset("ThreadLocalAllocBuffer::_slow_allocations",<br>
> Integer.class, "unsigned");<br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> private final int threadLocalAllocBufferFastRefillWasteOffset<br>
> = getFieldOffset("ThreadLocalAllocBuffer::_fast_refill_waste",<br>
> Integer.class, "unsigned");<br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> private final int threadLocalAllocBufferNumberOfRefillsOffset<br>
> = getFieldOffset("ThreadLocalAllocBuffer::_number_of_refills",<br>
> Integer.class, "unsigned");<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>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> private final int threadLocalAllocBufferDesiredSizeOffset =<br>
> getFieldOffset("ThreadLocalAllocBuffer::_desired_size",<br>
> Integer.class, "size_t");<br>
> org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java: <br>
> public final int tlabAlignmentReserve =<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>>> 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 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 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>
>> 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 <<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. However,<br>
>>> with the change touching quite a bit of various code 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 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 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 sanity<br>
>>> tests to ensure I had not missed anything.<br>
>>> Thanks for your help and I hope this is not too much 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> <<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? Should it<br>
>>> just be hotspot-compiler-dev?<br>
> <br>
</blockquote></div>