<div dir="ltr">Small update:<div><br></div><div>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).</div><div><a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.02/">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.02/</a><br></div><div><br></div><div>Let me know what you think,</div><div>Jc</div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 3:21 PM JC Beyler <<a href="mailto:jcbeyler@google.com">jcbeyler@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi Karen and Stefan,<br><div><br></div><div>@Stefan: Naming is hard :)</div><div>@Karen: thanks for the Graal comment, I fixed it in the new webrev, let me know what you think :)<br></div><div><br></div><div>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:</div><div><br></div><div>_end -> the _end or sampling end</div><div>_allocation_end -> end pointer for the last possible allocation</div><div>hard_end -> allocation end + reserved space</div><div><br></div><div>That is how this naming came up and why hard_end went to "reserved_end".</div><div><br></div><div>I'm really indifferent, so I offer as a perusal:</div><div><a href="http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.01/" target="_blank">http://cr.openjdk.java.net/~jcbeyler/8201326/webrev.01/</a><br></div><div><br></div><div>I noticed a few problems of alignement of '{' so I'll go fix that. Basically this webrev does the following:</div><div><br></div><div>- Uses fast_path_end instead of end</div><div>- Reverts hard_end back to where it was</div><div>- Adds the changes to Graal; there is a bunch of changes in Graal because Graal still contains a bit of code doing fasttlabrefills.</div><div><br></div><div>Let me know what you think!</div><div><br></div><div>Thanks,</div><div>Jc</div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr">On Tue, Apr 10, 2018 at 6:56 AM Karen Kinnear <<a href="mailto:karen.kinnear@oracle.com" target="_blank">karen.kinnear@oracle.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space">Hi JC,<div><br></div><div>A comment about Graal. The impact on Graal for this particular change would be to break it - so you’ll need</div><div>to complete the Graal changes for this renaming. That isn’t optional or something that could be a follow-on. It</div><div>is not ok to break a feature, even an experimental one. We will discuss in the other thread potential phasing of adding sampling.</div><div><br></div><div>I did not do a thorough search -you can do that - I did find </div><div>src/jdk.internal.vm.compiler/share/classes/</div><div><div style="margin:0px;font-stretch:normal;font-size:11px;line-height:normal;font-family:Menlo;background-color:rgb(255,255,255)"><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    public final int threadTlabOffset = getFieldOffset("Thread::_tlab", Integer.class, "ThreadLocalAllocBuffer");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferStartOffset = getFieldOffset("ThreadLocalAllocBuffer::_start", Integer.class, "HeapWord*");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferEndOffset = getFieldOffset("ThreadLocalAllocBuffer::_end", Integer.class, "HeapWord*");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferTopOffset = getFieldOffset("ThreadLocalAllocBuffer::_top", Integer.class, "HeapWord*");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferPfTopOffset = getFieldOffset("ThreadLocalAllocBuffer::_pf_top", Integer.class, "HeapWord*");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferSlowAllocationsOffset = getFieldOffset("ThreadLocalAllocBuffer::_slow_allocations", Integer.class, "unsigned");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferFastRefillWasteOffset = getFieldOffset("ThreadLocalAllocBuffer::_fast_refill_waste", Integer.class, "unsigned");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferNumberOfRefillsOffset = getFieldOffset("ThreadLocalAllocBuffer::_number_of_refills", Integer.class, "unsigned");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferRefillWasteLimitOffset = getFieldOffset("ThreadLocalAllocBuffer::_refill_waste_limit", Integer.class, "size_t");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/GraalHotSpotVMConfig.java:    private final int threadLocalAllocBufferDesiredSizeOffset = getFieldOffset("ThreadLocalAllocBuffer::_desired_size", Integer.class, "size_t");</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">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”);</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures"><br></span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">hope this helps,</span></div><div style="margin:0px;font-stretch:normal;line-height:normal"><span style="font-variant-ligatures:no-common-ligatures">Karen</span></div></div><div><br><blockquote type="cite"><div>On Apr 10, 2018, at 7:04 AM, Stefan Johansson <<a href="mailto:stefan.johansson@oracle.com" target="_blank">stefan.johansson@oracle.com</a>> wrote:</div><br class="m_8096277297514539933m_7386594880489431749Apple-interchange-newline"><div><div>Hi JC,<br><br>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:<br>  cmpptr(end, Address(thread, 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 this rename I think you can re-use _end in JEP-331 instead of calling it _allocation_end. But that is a later review :)<br><br>Also, is there a good reason for renaming hard_end() to reserved_end()?<br><br>One additional thing, you need to update the SA to reflect 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><blockquote type="cite">Hi all,<br>Small pre-amble to this request:<br>In my work to try to get a heap sampler in OpenJDK (via JEP 331 <<a href="https://bugs.openjdk.java.net/browse/JDK-8171119" target="_blank">https://bugs.openjdk.java.net/browse/JDK-8171119</a>>), 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".<br>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.<br>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 :)<br>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.<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/" 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 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 not sure <a href="https://teams.googleplex.com/u/where" target="_blank">who/where</a> 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?<br></blockquote></div></div></blockquote></div><br></div></div></blockquote></div></blockquote></div>