Skip to content

Fix for Gctoolkit #372 - YoungDetails event not published for GC log collected with -Xlog:gc (no *)#430

Merged
kcpeppe merged 2 commits intomicrosoft:mainfrom
jlittle-ptc:gctoolkit-372
Apr 21, 2025
Merged

Fix for Gctoolkit #372 - YoungDetails event not published for GC log collected with -Xlog:gc (no *)#430
kcpeppe merged 2 commits intomicrosoft:mainfrom
jlittle-ptc:gctoolkit-372

Conversation

@jlittle-ptc
Copy link
Contributor

Approach:

Both files that are generated using -Xlog:gc and -Xlog:gc* use the same general format for the log line of interest:
-Xlog:gc: [6.837s][info][gc] GC(3) Pause Young (Normal) (G1 Evacuation Pause) 79M->27M(1024M) 2.929ms
-Xlog:gc*: [1.361s][info ][gc ] GC(0) Pause Young (Normal) (G1 Evacuation Pause) 18874M->9037M(81920M) 23.698ms

The main difference is that for -Xlog:gc, this is the only line that is printed for an event, whereas -Xlog:gc* prints a CPU time line right afterward, which GCToolkit currently uses as a publish trigger:
[1.361s][info ][gc,cpu ] GC(0) User=0.95s Sys=0.09s Real=0.02s

To address this issue, I've added a Diarizer flag PRINT_CPU_TIMES that is populated when extracting decorators. If this flag is unset/unsure, the UnifiedG1GCParser.youngDetails() method will populate remaining fields (GC Type - Young, GC Cause extracted from trace 4 (offset -2), StartTime as reported by getClock()) and attempt to publish the event. I have left in a null check on GCType in the publish condition as a failsafe, but in theory it could be removed.

I think this should be reasonably solid, is a relatively minor change, and it will keep events being published in order.

Test Updates
Not much done here, other than adding a couple of updates to existing test cases. I can provide a sample file, and can update some additional suites, though I wanted to confirm how I can go about doing that. Is it just a matter of a PR to gctoolkit-testdata with the new file first?

@kcpeppe
Copy link
Contributor

kcpeppe commented Apr 21, 2025

This looks good, thank you! There is additional work that needs to be done but I'll create a new issue to describe that. As for test data, yes, you will need to add the log files to gctoolkit-testdata. Once that is in, I'll trigger a release.

Copy link
Contributor

@kcpeppe kcpeppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a follow on issue to fix diarization tests for the Generational, Z, and Shenandoah collectors.

@kcpeppe kcpeppe merged commit e226885 into microsoft:main Apr 21, 2025
@karianna karianna mentioned this pull request Apr 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants