Hi Jc, sorry for taking a bit long to respond.... ;)
On Mon, 2017-10-23 at 08:27 -0700, JC Beyler wrote: > Dear all, > > Small update this week with this new webrev: > - http://cr.openjdk.java.net/~rasbold/8171119/webrev.13/ > - Incremental is here: http://cr.openjdk.java.net/~rasbold/8171119/ > webrev.12_13/ > > I patched the code changes showed by Robbin last week and I > refactored collectedHeap.cpp: > http://cr.openjdk.java.net/~rasbold/8171119/webrev.12_13/src/hotspot/ > share/gc/shared/collectedHeap.cpp.patch > > The original code became a bit too complex in my opinion with the > handle_heap_sampling handling too many things. So I subdivided the > logic into two smaller methods and moved out a bit of the logic to > make it more clear. Hopefully it is :) > > Let me know if you have any questions/comments :) > Jc A few minor issues: - weak reference handling has been factored out in JDK-8189359, now you only need to add the additions required for this change to one place. :) Please update the webrev :) - the one issue Robin noticed. - in the declaration of CollectedHeap::sample_allocation, it would be nice if the fix_sample_rate parameter would be described - it takes a time to figure out what it's used for. I.e. in case an allocation goes beyond the sampling watermark, this value which represents the amount of overallocation is used to adjust the next sampling watermark to sample at the correct rate. Something like this - and if what I wrote is incorrect, there is even more reason to document it. Or maybe just renaming "fix_sample_rate" to something more descriptive - but I have no good idea about that. With lack of units in the type, it would also be nice to have the unit in the identifier name, as done elsewhere. - some (or most actually) of the new setters and getters in the ThreadLocalAllocBuffer class could be private I think. Also, we typically do not use "simple" getters that just return a member in the class where they are defined. - ThreadLocalAllocBuffer::set_sample_end(): please use pointer_delta() for pointer subtractions. - ThreadLocalAllocBuffer::pick_next_sample() - I recommend making the first check an assert - it seems that it is only useful to call this with heap monitoring enabled, as is done right now. - ThreadLocalAllocBuffer::pick_next_sample() - please use "PTR_FORMAT" (or INTPTR_FORMAT - they are the same) as format string for printing pointer values as is customary within Hotspot. %p output is OS dependent. I.e. I heard that e.g. on Ubuntu it prints "null" instead of 0x0...0 .... which is kind of annoying. - personal preference: do not allocate HeapMonitoring::AlwaysTrueClosure globally, but only locally when it's used. Setting it up seems to be very cheap. - HeapMonitoring::next_random() - the different names for the constants use different formatting. Preferable (to me) is UpperCamelCase, but at least make them uniform. - in HeapMonitoring::next_random(), you might want to use right_n_bits() to create your mask. - not really convinced that it is a good idea to not somehow guard StartHeapSampling() and StopHeapSampling() against being called by multiple threads. Otherwise looks okay from what I can see. Thanks, Thomas
