#433 - Fixes and tests for Pre-Unified G1GC and Parallel

[jlittle-ptc]

Fixes:

• GenerationalHeapParser.psYoungNoDetails() - Offsets were off by 3.
  • Note: I didn't have any sample lines for a full GC, but I suspect the same issue will apply there. I did not make the change at this time, but left a note in a comment.
• PreUnifiedG1GCParser.processYoungGenCollection() - Offset for initial-mark was off by 3.

Test Cases:

• Event parsing test cases for Pre-unified G1 and Parallel/CMS with Details/NoDetails

[kcpeppe]

Triggering a System.gc() via jcmd (or JMX or ...) would yield the missing Full GC record.

[dsgrieve]

@jlittle-ptc - Do you have a gclog that exhibits this behavior? If so, can you attach it here and we can add it to the gclogs. Thanks.

[Copilot]

Pull Request Overview

This PR fixes an off-by-three error in parsing GC log offsets for Pre-Unified G1GC events and generational collection events while introducing comprehensive tests for Pre-unified G1 and Parallel/CMS logs.

• Fixes off-by-three errors in GC log group offsets for both young and full GC events.
• Adds new test cases in PreUnifiedG1GCParserTest.java, GenerationalHeapParserTest.java, and updates ParserTest.java and G1GCParserRulesTest.java.
• Adjusts offset indices in PreUnifiedG1GCParser.java and GenerationalHeapParser.java to correct GC event parsing.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
parser/src/test/java/com/microsoft/gctoolkit/parser/PreUnifiedG1GCParserTest.java	New test cases validating Pre-Unified G1GC events and their memory/CPU stats.
parser/src/test/java/com/microsoft/gctoolkit/parser/ParserTest.java	Added helper methods for survivor and CPU summary assertions.
parser/src/test/java/com/microsoft/gctoolkit/parser/GenerationalHeapParserTest.java	Extended tests covering various generational GC events including PSYoungNoDetails and SystemGC.
parser/src/test/java/com/microsoft/gctoolkit/parser/G1GCParserRulesTest.java	Revised GC parse rule tests with adjusted constants.
parser/src/main/java/com/microsoft/gctoolkit/parser/PreUnifiedG1GCParser.java	Adjusted group offsets in processYoungGenCollection() and processYoung() methods to address off-by-three errors.
parser/src/main/java/com/microsoft/gctoolkit/parser/GenerationalHeapParser.java	Updated offset indices in psYoungNoDetails() to align with the changes from issue #433.

[Copilot]

The updated offset in creating the G1YoungInitialMark instance appears justified by issue #433; please ensure that similar adjustments across the parser maintain consistency and consider referencing the issue for future maintainers.

Suggested change

	// jlittle-ptc: Cause was misaligned, originally pointed to group 3, but seems to be consistently in group 6.
	// which is default for trace.gcCause(), and would match with other off-by-3 issues I've found.
	// Issue #433: Cause was misaligned, originally pointed to group 3, but seems to be consistently in group 6.
	// This adjustment ensures consistency with other similar cases and addresses the off-by-3 issue.

[Copilot]

[nitpick] The offset change from 9 to 12 for retrieving memory pool values should be accompanied by a brief comment or reference to issue #433 to explain the rationale and aid future maintenance.

Suggested change

	// #433 - All offsets in method incremented by 3 as they weren't matching the correct groups.
	// #433 - All offsets in method incremented by 3 as they weren't matching the correct groups.
	// Offset changed from 9 to 12 to correctly retrieve memory pool values. See issue #433 for details.

[Copilot]

[nitpick] A comment referencing issue #433 would clarify why the offset was changed from 4 to 7, ensuring consistency and easing future troubleshooting.

Suggested change

	SystemGC collection = new SystemGC(trace.getDateTimeStamp(), GCCause.JAVA_LANG_SYSTEM, trace.getDuration());
	SystemGC collection = new SystemGC(trace.getDateTimeStamp(), GCCause.JAVA_LANG_SYSTEM, trace.getDuration());
	// #433 - Offset for this call needed to be incremented by 3. (previously 4)

[jlittle-ptc]

@jlittle-ptc - Do you have a gclog that exhibits this behavior? If so, can you attach it here and we can add it to the gclogs. Thanks.

gc.txt

This file was attached to #433, where the issue was reported.

Once I get closer to finishing my review, I'll look to upload any examples that I have that are different than what's in the test data.

[jlittle-ptc]

Triggering a System.gc() via jcmd (or JMX or ...) would yield the missing Full GC record.

I managed to capture a sample FullGC and verified that psFull had the same issue. Updated the offset and added a test case for it.

[dsgrieve]

gc.txt

This file was attached to #433, where the issue was reported.

Once I get closer to finishing my review, I'll look to upload any examples that I have that are different than what's in the test data.
Thanks! We put these files in https://github.com/microsoft/gctoolkit-testdata. To use them in gctoolkit, we run a github release on gctoolkit-testdata and then update the gclogs/pom.xml file for the new gctoolkit-testdata version.