<div dir="ltr">Hi all,<div><br></div><div>First off: Thanks again to Robbin and Thomas for their reviews :)</div><div><br></div><div>Next, I've uploaded a new webrev:</div><div><a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/">http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/</a></div><div><br></div><div>Here is an update:</div><div><br></div><div>- @Robbin, I forgot to say that yes I need to look at implementing this for the other architectures and testing it before it is all ready to go. Is it common to have it working on all possible combinations or is there a subset that I should be doing first and we can do the others later?</div><div>- I've tested slowdebug, built and ran the JTreg tests I wrote with slowdebug and fixed a few more issues<br></div><div>- I've refactored a bit of the code following Thomas' comments<br></div><div>   - I think I've handled all the comments from Thomas (I put comments inline below for the specifics)</div><div><br></div><div>The biggest addition to this webrev is that there is more testing, I've added two tests for looking specifically at the two garbage sampling data Recent vs Frequent:</div><div>   <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java.patch</a> </div><div>   and</div><div>   <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java.patch</a></div><div><br></div><div>I've also refactored the JNI library a bit: <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch</a>.</div><div>   - Side question: I was looking at trying to make this a multi-file library so you would not have all the code in one spot. It seems this is not really done?</div><div>      - Is there a solution to this? What I really wanted was:</div><div>        - The core library that will help testing be easier</div><div>        - The JNI code for each Java class separate and calling into the core library</div><div><br></div><div>- Following Thomas' comments on statistics, I want to add some quality assurance tests and find that the easiest way would be to have a few counters of what is happening in the sampler and expose that to the user.</div><div>   - I'll be adding that in the next version if no one sees any objections to that.</div><div>   - This will allow me to add a sanity test in JTreg about number of samples and average of sampling rate</div><div><br></div><div>@Thomas: I had a few questions that I inlined below but I will summarize the "bigger ones" here:</div><div>   - You mentioned constants are not using the right conventions, I looked around and didn't see any convention except normal naming then for static constants. Is that right?</div><div>   - You mentioned putting the weak_oops_do in a separate method and logging, I inlined my answer below and would appreciate your feedback there too.</div><div><br></div><div>Thanks again for your reviews and patience!<br></div><div>Jc</div><div><br></div><div>PS: I've also inlined my answers to Thomas below:</div><div><br></div><div><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 13, 2017 at 8:03 AM, Thomas Schatzl <span dir="ltr"><<a href="mailto:thomas.schatzl@oracle.com" target="_blank">thomas.schatzl@oracle.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi all,<br>
<span class="gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-"><br>
On Mon, 2017-06-12 at 11:11 -0700, JC Beyler wrote:<br>
> Dear all,<br>
><br>
</span><span class="gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-">> I've continued working on this and have done the following webrev:<br>
> <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.05/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~ra<wbr>sbold/8171119/webrev.05/</a><br>
<br>
</span>[...]<br>
<span class="gmail-m_-1455798157861946755gmail-m_7132783609556290314gmail-">> Things I still need to do:<br>
>    - Have to fix that TLAB case for the FastTLABRefill<br>
>    - Have to start looking at the data to see that it is consistent<br>
> and does gather the right samples, right frequency, etc.<br>
>    - Have to check the GC elements and what that produces<br>
>    - Run a slowdebug run and ensure I fixed all those issues you saw<br>
> Robbin<br>
><br>
> Thanks for looking at the webrev and have a great week!<br>
<br>
</span>  scratching a bit on the surface of this change, so apologies for<br>
rather shallow comments:<br>
<br>
- macroAssembler_x86.cpp:5604: while this is compiler code, and I am<br>
not sure this is final, please avoid littering the code with TODO<br>
remarks :) They tend to be candidates for later wtf moments only.<br>
<br>
Just file a CR for that.<br>
<br></blockquote><div><br></div><div>Newcomer question: what is a CR and not sure I have the rights to do that yet ? :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- calling HeapMonitoring::do_wea<wbr>k_oops() (which should probably be<br>
called weak_oops_do() like other similar methods) only if string<br>
deduplication is enabled (in g1CollectedHeap.cpp:4511) seems wrong.</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
The call should be at least around 6 lines up outside the if.<br>
<br>
Preferentially in a method like process_weak_jni_handles(), including<br>
additional logging. (No new (G1) gc phase without minimal logging :)).<br></blockquote><div><br></div><div>Done but really not sure because:<br></div><div><br></div><div>I put for logging:</div><div><div>  log_develop_trace(gc, freelist)("G1ConcRegionFreeing [other] : heap monitoring");</div></div><div><br></div><div>Since weak_jni_handles didn't have logging for me to be inspired from, I did that but unconvinced this is what should be done.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Seeing the changes in referenceProcess.cpp, you need to add the call to<br>
HeapMonitoring::do_weak_oops() exactly similar to<br>
process_weak_jni_handles() in case there is no reference processing<br>
done.<br>
<br>
- psParallelCompact.cpp:2172 similar to other places where the change<br>
adds the call to HeapMonitoring::do_weak_oops()<wbr>, remove the empty line.<br></blockquote><div><br></div><div>Done from what I can tell in the whole webrev. (The only empty lines I still see are when I maintain an empty line, exception being <a href="http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/src/share/vm/gc/shared/collectedHeap.cpp.patch">http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/src/share/vm/gc/shared/collectedHeap.cpp.patch</a> where I think it was "nicer")</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- the change doubles the size of<br>
CollectedHeap::allocate_from_t<wbr>lab_slow() above the "small and nice"<br>
threshold. Maybe it could be refactored a bit.<br></blockquote><div><br></div><div>Done I think, it looks better to me :).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- referenceProcessor.cpp:40: please make sure that the includes are<br>
sorted alphabetically (I did not check the other files yet).<br></blockquote><div><br></div><div>Done and checked all other files normally.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- referenceProcessor.cpp:261: the change should add logging about the<br>
number of references encountered, maybe after the corresponding "JNI<br>
weak reference count" log message.<br></blockquote><div><br></div><div>Just to double check, are you saying that you'd like to have the heap sampler to keep in store how many sampled objects were encountered in the HeapMonitoring::weak_oops_do?</div><div>   - Would a return of the method with the number of handled references and logging that work?</div><div>   - Additionally, would you prefer it in a separate block with its GCTraceTime?</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- threadLocalAllocBuffer.cpp:3<wbr>31: one more "TODO"<br></blockquote><div><br></div><div>Removed it and added it to my personal todos to look at.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- threadLocalAllocBuffer.hpp: ThreadLocalAllocBuffer class<br>
documentation should be updated about the sampling additions. I would<br>
have no clue what the difference between "actual_end" and "end" would<br>
be from the given information.<br></blockquote><div><br></div><div>If you are talking about the comments in this file, I made them more clear I hope in the new webrev. If it was somewhere else, let me know where to change.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- threadLocalAllocBuffer.hpp:130<wbr>: extra whitespace ;)<br></blockquote><div><br></div><div>Fixed :)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- some copyrights :)<br>
<br></blockquote><div><br></div><div>I think I fixed all the issues, if you see specific issues, let me know.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
- in heapMonitoring.hpp: there are some random comments about some code<br>
that has been grabbed from "util/math/fastmath.[h|cc]". I can't tell<br>
whether this is code that can be used but I assume that Noam Shazeer is<br>
okay with that (i.e. that's all Google code).<br></blockquote><div><br></div><div>Jeremy and I double checked and we can release that as I thought. I removed the comment from that piece of code entirely.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- heapMonitoring.hpp/cpp static constant naming does not correspond to<br>
Hotspot's. Additionally, in Hotspot static methods are cased like other<br>
methods.<br></blockquote><div><br></div><div>I think I fixed the methods to be cased the same way as all other methods. For static constants, I was not sure. I fixed a few other variables but I could not seem to really see a consistent trend for constants. I made them as variables but I'm not sure now.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
- in heapMonitoring.cpp there are a few cryptic comments at the top<br>
that seem to refer to internal stuff that should probably be removed.<br></blockquote><div><br></div><div>Sorry about that! My personal todos not cleared out.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I did not think through the impact of the TLAB changes on collector<br>
behavior yet (if there are). Also I did not check for problems with<br>
concurrent mark and SATB/G1 (if there are).<br></blockquote><div><br></div><div>I would love to know your thoughts on this, I think this is fine. I see issues with multiple threads right now hitting the stack storage instance. Previous webrevs had a mutex lock here but we took it out for simplificity (and only for now).</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks,<br>
  Thomas<br>
<br>
</blockquote></div><br></div></div></div>