RFR: 8241361: ZGC: Implement memory related JFR events

[Stefan Karlsson]

On 2020-03-26 12:01, Per Liden wrote:
> 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?

Overflushing causes malloc calls for pages and bitmaps. I thought that 
could be of interest when looking at latencies. If you don't want it, 
I'll remove it.

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

OK.

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

What I'm meaning is that if you have the minimally sufficient 
information like:
- Relocated bytes in small pages: x
- Relocated bytes in medium pages: y
- Relocated bytes in large pages: z

You need to go through extra lengths to figure out:
- Relocated total bytes: x + y + x

and unless you want to fiddle around with JMC too much, it's better to 
also (or instead) reported the sum of the values.

StefanK

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