RFR: 8241361: ZGC: Implement memory related JFR events

[Per Liden]

Hi,

On 3/23/20 10:30 AM, Stefan Karlsson wrote:
[...]
>>
>> * 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?

Hmm, not sure I see the point of reporting anything except what was 
actually flushed. When would the other numbers be of interest? Keep in 
mind that the overflushed part of this is immediately put back into the 
cache, and is never unmapped/destroyed or anything like that. Outside of 
flush_cache() no one will know (or care) if we overflushed or not, right?

> 
>>
>>
>> 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.

In this case, I'd prefer getters. Assuming my comment above is accepted, 
we only need one new getter.

> 
>>
>> 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.

Ok, I'm sure you're right but still want to understand. When you say 
"normalized events", what are you thinking of in this context?

cheers,
Per

> 
>>
>>
>> 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.
>