-
Notifications
You must be signed in to change notification settings - Fork 156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ZGC Generational Support #414
Conversation
This patch adds support for generational ZGC and maintains support for legacy (non generational ZGC). There are a number of potential API breaking changes for consumers of GC events. List of changes and modifications are listed below. 1. Introduce new GCEvent types, FullZGCCycle (legacy), MajorZGCCycle and MinorZGCCycle representing the respective types. 2. GCEvents Full|Major|Minor GC Cycles hold reference to internal ZGCCycle, a delegate in the case of Full (legacy) cycle, young and for Major cycle and simply a young for Minor cycle. All cycle types full/young/old, have mostly similar overlapping fields, which allows us to have a single data class for all cycle types. Cycles themselves have a few fields that are specific to entire cycle and are held in the top level object. ``` ┌─────────────┐ │ Full │ │ │ ┌────────┐ │ delegate ──┼─────▶│ZGCCycle│ │ │ └────────┘ └─────────────┘ ┌─────────────┐ │ Major │ │ │ ┌────────┐ │ young ───┼─────▶│ZGCCycle│ │ │ ├────────┤ │ old ───┼─────▶│ZGCCycle│ └─────────────┘ └────────┘ ┌─────────────┐ │ Minor │ │ │ ┌────────┐ │ young ───┼────▶│ZGCCycle│ │ │ └────────┘ └─────────────┘ ``` 3. Add stronger typing on the heap summary report. Live, Garbage, Allocated, Reclaimed, Promoted and Compacted carry information for a different stages of the GC. Each holds a different number of events. Having a single summary with primitive `double` doesn't make sense as you can't differentiate between 0 and non present. We could use optional or boxed type but it seemed more direct to declare a type. In future java versions these could simply be Record classes. 4. Added concept of phase to the ZGC Parsers. The phase represents whether the GC cycle is in Major young/old, Minor young or Full phase (legacy). This maps to the GC output of 'Y', 'y', 'O' prefix on the log line. We key off this to make a determination about which heap statistics we are writing to. Phase is determined by an optional capture group at the head of all log lines. 5. Introduced new hierarchical forward reference type to allow it to cary more contextual information about either young or old generation. Major obviously has two references, one for old and one for young. Minor and full GC's cary a single forward ref corresponding to their cycle. 6. Added new ZGC generational test for both Minor and Major cycle types ensuring that both are parsed correctly.
Adds missing GC cause, based on enum toString defined in `jdk/src/hotspot/share/gc/shared/gcCause.cpp`
👍 |
Thanks for this PR! It may take us a day or two to pick through it, but it's appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the very high quality patch. We will release this ASAP, likely tomorrow (Feb 26). Please create a PR for any comments you would like to address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry if this is pedantic but occupancyBefore and occupancyAfter are more meaningful than start and end. I believe this is consistent with the other MemoryPool. Also, the other MemoryPool abstractions contain the size of the pool. In most cases this is the current size of Java heap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense. The memory summary was meant to capture the closing line on any of the cycles eg
GC(40) Garbage Collection (Proactive) 698M(2%)->262M(1%)
where the occupancy was 698M before and 262M after.
There are other other log lines that convey the capacity of the heap which could be combined to form a complete summary. I'm happy to attempt to implement this.
GC(40) Min Capacity: 8M(0%)
GC(40) Max Capacity: 30718M(100%)
GC(40) Soft Max Capacity: 30718M(100%)
@@ -289,4 +302,13 @@ public void notYetImplemented() { | |||
System.out.println("-----------------------------------------"); | |||
//} | |||
} | |||
|
|||
public ZGCPhase getZCollectionPhase() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A GCLogTrace is a convenience wrapper over the Matcher. It contains convenience methods to extract data from the matcher. It is expected that the parser has the context to know which methods to call and which groups need to be extracted. Thus ZGCPhase is either known and extracted by the parser at that time or ZGCPhase isn't present in the log line (matcher) and the parser, again, should know this and not call this method. Either way, ZGCPhase should be consistent and there should be no need for downstream exception handling. This is something that was noted in ZGCPhase that an illegal argument was being thrown which likely is unnecessary.
@@ -85,6 +89,15 @@ public double getMilliseconds(int index) { | |||
return getDoubleGroup(index); | |||
} | |||
|
|||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parser should know this.. and it should be picked up in testing
return status; | ||
} | ||
} | ||
throw new IllegalArgumentException("No matching ZGCCollectionType found for: " + label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should never get here.
// Licensed under the MIT License. | ||
package com.microsoft.gctoolkit.event.zgc; | ||
|
||
public class ZGCGarbageSummary { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these values heap occupancy before or after the collection?
|
||
public ZGCMemorySummary(long start, long end) { | ||
this.start = start; | ||
this.end = end; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
start and end are ambiguous, can these names be more descriptive?
return zgcPhase; | ||
} | ||
} | ||
throw new IllegalArgumentException("No matching ZGCPhase found for: " + label); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should never get here
@@ -20,6 +20,7 @@ public interface GenericTokens { | |||
//Time | |||
String TIME = "(-?" + REAL_NUMBER + ")"; | |||
String DURATION_MS = TIME + "\\s?ms"; | |||
String DURATION_S = TIME + "\\s?s"; | |||
String INT_DURATION_MS = INTEGER + "ms"; | |||
//0.0700188 | |||
String PAUSE_TIME = TIME + "\\s?(?:secs?|ms)"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have added the s to the PAUSE_TIME pattern but that is just me.
@@ -4,6 +4,9 @@ | |||
|
|||
import com.microsoft.gctoolkit.event.jvm.JVMEvent; | |||
import com.microsoft.gctoolkit.event.shenandoah.ShenandoahCycle; | |||
import com.microsoft.gctoolkit.event.zgc.OccupancySummary; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these changes intentional or should they be reverted? The added methods are never called?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this was a rebase error. I'll revert.
GCEvent getGCEVent(DateTimeStamp endTime); | ||
} | ||
|
||
private static class ZGCMinorForwardReference implements ZFwdRef { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would prefer if this was ZForwardReference instead of ZFwdRef
@j-bahr Thank you for your contribution which is now in gctoolkit 3.5.0 on Maven central. |
@kcpeppe, Thanks for the review I will incorporate feedback into another PR. |
Minor fixes from feedback in microsoft#414 - Fix enum types to avoid having to throw IllegalArgumentException - Fix Memory summary to have more descriptive variable names - ZGCPhase now handles the regex null case correctly for Full (legacy) phase types - Use pause time to collect optional GC duration - Rename ZFwdRef -> ZForwardReference - Remove unused methods in ShenandoahParserTest.java
Minor fixes from feedback in microsoft#414 - Fix enum types to avoid having to throw IllegalArgumentException - Fix Memory summary to have more descriptive variable names - ZGCPhase now handles the regex null case correctly for Full (legacy) phase types - Use pause time to collect optional GC duration - Rename ZFwdRef -> ZForwardReference - Remove unused methods in ShenandoahParserTest.java
Minor fixes from feedback in #414 - Fix enum types to avoid having to throw IllegalArgumentException - Fix Memory summary to have more descriptive variable names - ZGCPhase now handles the regex null case correctly for Full (legacy) phase types - Use pause time to collect optional GC duration - Rename ZFwdRef -> ZForwardReference - Remove unused methods in ShenandoahParserTest.java
This patch adds support for generational ZGC and maintains support for legacy (non generational ZGC). There are a number of potential API breaking changes for consumers of GC events. List of changes and modifications are listed below.
double
doesn't make sense as you can't differentiate between 0 and not present. We could use optional or boxed type but it seemed more direct to declare a type. In future java versions these could simply be Record classes.Y
,y
,O
prefix on the log line. We key off this to make a determination about which heap statistics we are writing to. Phase is determined by an optional capture group at the head of all log lines.