Low-Overhead Heap Profiling
jcbeyler at google.com
Wed Jun 21 20:45:27 UTC 2017
First off: Thanks again to Robbin and Thomas for their reviews :)
Next, I've uploaded a new webrev:
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'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:
I've also refactored the JNI library a bit:
- 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
- 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
- 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
Thanks again for your reviews and patience!
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>
> 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/
> > 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
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
> - 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
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
- 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
> - 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
> - threadLocalAllocBuffer.hpp:130: extra whitespace ;)
> - 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
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).
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev