Low-Overhead Heap Profiling
thomas.schatzl at oracle.com
Tue Jun 13 15:03:46 UTC 2017
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:
> 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
> 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.
- 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 :)).
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.
- the change doubles the size of
CollectedHeap::allocate_from_tlab_slow() above the "small and nice"
threshold. Maybe it could be refactored a bit.
- referenceProcessor.cpp:40: please make sure that the includes are
sorted alphabetically (I did not check the other files yet).
- referenceProcessor.cpp:261: the change should add logging about the
number of references encountered, maybe after the corresponding "JNI
weak reference count" log message.
- threadLocalAllocBuffer.cpp:331: one more "TODO"
- 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.
- threadLocalAllocBuffer.hpp:130: extra whitespace ;)
- some copyrights :)
- 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).
- heapMonitoring.hpp/cpp static constant naming does not correspond to
Hotspot's. Additionally, in Hotspot static methods are cased like other
- in heapMonitoring.cpp there are a few cryptic comments at the top
that seem to refer to internal stuff that should probably be removed.
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).
More information about the hotspot-compiler-dev