Low-Overhead Heap Profiling

[Robbin Ehn]

Hi,

On 06/21/2017 10:45 PM, JC Beyler wrote:
> Hi all,
> 
> First off: Thanks again to Robbin and Thomas for their reviews :)
> 
> Next, I've uploaded a new webrev:
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/
> 
> Here is an update:
> 
> - @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?

I will change my mind here and proposed a different approach.
Right now the only compiler platform change you have is for the FastTLABRefill.
FastTLABRefill works for all gc except G1 (CMS is deprecated) but only in C1 compiled code.
The performance angle of this is not really relevant, since then you will have C2 compiled code.

So I suggested that we should remove FastTLABRefill completely, I will try to makes this happen but most likely not before my long vacation.

That means you can avoid all platform specific code (as patch is now), correct?

If so just keep the x86 support in there and we remove that if I succeed, plus you can support all platforms when FastTLABRefill=false.

Sounds good?

If you had any other questions I missed please let me know.
I'll look at your latest webrev, and response to that also.

Thanks!

/Robbin


> - I've tested slowdebug, built and ran the JTreg tests I wrote with slowdebug and fixed a few more issues
> - I've refactored a bit of the code following Thomas' comments
>     - I think I've handled all the comments from Thomas (I put comments inline below for the specifics)
> 
> 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:
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorRecentTest.java.patch
>     and
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/MyPackage/HeapMonitorFrequentTest.java.patch
> 
> I've also refactored the JNI library a bit: http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/test/serviceability/jvmti/HeapMonitor/libHeapMonitor.c.patch.
>     - 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?
>        - Is there a solution to this? What I really wanted was:
>          - The core library that will help testing be easier
>          - The JNI code for each Java class separate and calling into the core library
> 
> - 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.
>     - I'll be adding that in the next version if no one sees any objections to that.
>     - This will allow me to add a sanity test in JTreg about number of samples and average of sampling rate
> 
> @Thomas: I had a few questions that I inlined below but I will summarize the "bigger ones" here:
>     - 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?
>     - 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.
> 
> Thanks again for your reviews and patience!
> Jc
> 
> PS: I've also inlined my answers to Thomas below:
> 
> On Tue, Jun 13, 2017 at 8:03 AM, Thomas Schatzl <thomas.schatzl at oracle.com <mailto:thomas.schatzl at oracle.com>> wrote:
> 
>     Hi all,
> 
>     On Mon, 2017-06-12 at 11:11 -0700, JC Beyler wrote:
>     > Dear all,
>     >
>     > I've continued working on this and have done the following webrev:
>     > http://cr.openjdk.java.net/~rasbold/8171119/webrev.05/ <http://cr.openjdk.java.net/~rasbold/8171119/webrev.05/>
> 
>     [...]
>     > Things I still need to do:
>     >    - Have to fix that TLAB case for the FastTLABRefill
>     >    - Have to start looking at the data to see that it is consistent
>     > and does gather the right samples, right frequency, etc.
>     >    - Have to check the GC elements and what that produces
>     >    - Run a slowdebug run and ensure I fixed all those issues you saw
>     > Robbin
>     >
>     > Thanks for looking at the webrev and have a great week!
> 
>        scratching a bit on the surface of this change, so apologies for
>     rather shallow comments:
> 
>     - macroAssembler_x86.cpp:5604: while this is compiler code, and I am
>     not sure this is final, please avoid littering the code with TODO
>     remarks :) They tend to be candidates for later wtf moments only.
> 
>     Just file a CR for that.
> 
> 
> Newcomer question: what is a CR and not sure I have the rights to do that yet ? :)
> 
>     - calling HeapMonitoring::do_weak_oops() (which should probably be
>     called weak_oops_do() like other similar methods) only if string
>     deduplication is enabled (in g1CollectedHeap.cpp:4511) seems wrong.
> 
> 
>     The call should be at least around 6 lines up outside the if.
> 
>     Preferentially in a method like process_weak_jni_handles(), including
>     additional logging. (No new (G1) gc phase without minimal logging :)).
> 
> 
> Done but really not sure because:
> 
> I put for logging:
>    log_develop_trace(gc, freelist)("G1ConcRegionFreeing [other] : heap monitoring");
> 
> 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.
> 
> 
>     Seeing the changes in referenceProcess.cpp, you need to add the call to
>     HeapMonitoring::do_weak_oops() exactly similar to
>     process_weak_jni_handles() in case there is no reference processing
>     done.
> 
>     - psParallelCompact.cpp:2172 similar to other places where the change
>     adds the call to HeapMonitoring::do_weak_oops(), remove the empty line.
> 
> 
> 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 
> http://cr.openjdk.java.net/~rasbold/8171119/webrev.06/src/share/vm/gc/shared/collectedHeap.cpp.patch where I think it was "nicer")
> 
> 
>     - the change doubles the size of
>     CollectedHeap::allocate_from_tlab_slow() above the "small and nice"
>     threshold. Maybe it could be refactored a bit.
> 
> 
> Done I think, it looks better to me :).
> 
> 
>     - referenceProcessor.cpp:40: please make sure that the includes are
>     sorted alphabetically (I did not check the other files yet).
> 
> 
> Done and checked all other files normally.
> 
> 
>     - referenceProcessor.cpp:261: the change should add logging about the
>     number of references encountered, maybe after the corresponding "JNI
>     weak reference count" log message.
> 
> 
> 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?
>     - Would a return of the method with the number of handled references and logging that work?
>     - Additionally, would you prefer it in a separate block with its GCTraceTime?
> 
> 
>     - threadLocalAllocBuffer.cpp:331: one more "TODO"
> 
> 
> Removed it and added it to my personal todos to look at.
> 
> 
>     - threadLocalAllocBuffer.hpp: ThreadLocalAllocBuffer class
>     documentation should be updated about the sampling additions. I would
>     have no clue what the difference between "actual_end" and "end" would
>     be from the given information.
> 
> 
> 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.
> 
> 
>     - threadLocalAllocBuffer.hpp:130: extra whitespace ;)
> 
> 
> Fixed :)
> 
> 
>     - some copyrights :)
> 
> 
> I think I fixed all the issues, if you see specific issues, let me know.
> 
>     - in heapMonitoring.hpp: there are some random comments about some code
>     that has been grabbed from "util/math/fastmath.[h|cc]". I can't tell
>     whether this is code that can be used but I assume that Noam Shazeer is
>     okay with that (i.e. that's all Google code).
> 
> 
> Jeremy and I double checked and we can release that as I thought. I removed the comment from that piece of code entirely.
> 
> 
>     - heapMonitoring.hpp/cpp static constant naming does not correspond to
>     Hotspot's. Additionally, in Hotspot static methods are cased like other
>     methods.
> 
> 
> 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.
> 
> 
>     - in heapMonitoring.cpp there are a few cryptic comments at the top
>     that seem to refer to internal stuff that should probably be removed.
> 
> 
> Sorry about that! My personal todos not cleared out.
> 
> 
>     I did not think through the impact of the TLAB changes on collector
>     behavior yet (if there are). Also I did not check for problems with
>     concurrent mark and SATB/G1 (if there are).
> 
> 
> 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).
> 
> 
>     Thanks,
>        Thomas
> 
>