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. - 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 done. - 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 methods. - 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). Thanks, Thomas
