RFR: 8241361: ZGC: Implement memory related JFR events

[Stefan Karlsson]

On 2020-03-23 09:53, Per Liden wrote:
> Hi,
>
> On 3/20/20 2:43 PM, Stefan Karlsson wrote:
>> Hi all,
>>
>> Please review this patch to add some memory related JFR events to ZGC.
>>
>> https://cr.openjdk.java.net/~stefank/8241361/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8241361
>
> Nice! Looks good overall, a few minor things:
>
>
> src/hotspot/share/gc/z/zPageAllocator.cpp
> -----------------------------------------
>
> * Instead of:
>
>   #include "jfrfiles/jfrEventClasses.hpp"
>
> I think we usually do:
>
>   #include "jfr/jfrEvents.hpp"

OK

>
>
> * I could live without the whitespace you added on line 631.
>
>  630   ZPageCacheFlushForAllocationClosure cl(requested);
>  631
>  632   const size_t flushed = flush_cache(&cl, true /* for_allocation

Yes

>
>
> * I think cl->_flushed user here:
>
>  604   event.commit(cl->_requested, cl->_flushed, for_allocation);
>
> should instead just be:
>
>  604   event.commit(cl->_requested, flushed, for_allocation);
>
> Right?

I intentionally used cl->_flushed since that describes how much we 
flushed including overflushed parts of pages. Maybe we should report 
both values? Maybe also rename the local variable flushed to destroyed?

>
>
> src/hotspot/share/gc/z/zPageCache.hpp
> -------------------------------------
>
> Instead of:
>
>   friend class ZPageAllocator;
>
> add a getter for requested()?
>

I also want _flushed, depending on the resolution of the above. I don't 
think its bad to friend our closures that are pure extensions to the 
"owning" class. I don't have a very strong opinion here, but gravitated 
towards a friend declaration to minimize the exposure of the 
implementation details. If you still want me to add getters, I'll do it.

>
> src/hotspot/share/gc/z/zRelocationSetSelector.cpp
> -------------------------------------------------
>
> * Same here, instead of:
>
>   #include "jfrfiles/jfrEventClasses.hpp"
>
> I think we should do:
>
>   #include "jfr/jfrEvents.hpp"

Yes

>
>
> * You don't think we should use ZPageTypeType that you introduced, and 
> send three different ZRelocationSet events, one for each page type? 
> Shouldn't this event also be timed, and sent from within 
> ZRelocationSetSelectorGroup::select()?

JMC is not always great at handling normalized events. If we want events 
per type I think we should add them in _addition_ to the event I added.

>
>
> src/hotspot/share/gc/z/zTracer.cpp
> ----------------------------------
>
>   43     writer.write("small");
>   44     writer.write_key(ZPageTypeMedium);
>   45     writer.write("medium");
>   46     writer.write_key(ZPageTypeLarge);
>   47     writer.write("large");
>
> How about "Small", "Medium" and "Large"? I could only find one other 
> place (in jfrStackTraceRepository.cpp) where names were given, and 
> those start with a capital letter.

OK

Here's the updated webrevs with the easy fixes:
  https://cr.openjdk.java.net/~stefank/8241361/webrev.02.delta/
  https://cr.openjdk.java.net/~stefank/8241361/webrev.02

Waiting for answers and comments to the rest.

Thanks,
StefanK

>
> cheers,
> Per
>
>
>>
>> Added events:
>>
>> ZAllocationStall - Record when we run out of heap memory and the Java 
>> threads stall, waiting for the GC to free up memory.
>>
>> ZPageAllocation - Updated the existing event to also record the 
>> duration of the event. Updated the event to only be reported if the 
>> allocation takes longer than 1 ms.
>>
>> ZPageCacheFlush - Record when the page cache needs to be flushed. 
>> This usually happens when we run out of a specific page size and have 
>> to detach the physical and virtual memory to materialize a new ZPage. 
>> We also flush pages when we uncommit memory.
>>
>> ZRelocationSet - Record information about the selected relocation set.
>>
>> ZUncommit - Record when we uncommit and hand back memory to the OS.
>>
>> The patch also contains some small cosmetic changes to existing 
>> events, whitespace fixes.