Skip to content
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 Fixes from #414 #417

Merged
merged 1 commit into from
Mar 3, 2025
Merged

ZGC Fixes from #414 #417

merged 1 commit into from
Mar 3, 2025

Conversation

j-bahr
Copy link
Collaborator

@j-bahr j-bahr commented Feb 28, 2025

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

@j-bahr j-bahr requested a review from kcpeppe February 28, 2025 00:03
@karianna karianna requested a review from dsgrieve February 28, 2025 00:31
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
}
}
throw new IllegalArgumentException("No matching ZGCCollectionType found for: " + label);
return Arrays.stream(ZGCCollectionType.class.getEnumConstants())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll accept this but... what do you think about introducing "Unknown" and return that instead of returning null?

return Arrays.stream(ZGCPhase.class.getEnumConstants())
.filter(phase -> Objects.equals(phase.getPhase(), label))
.findFirst()
.orElse(null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as previous comment about Unknown

@kcpeppe kcpeppe merged commit 2792437 into microsoft:main Mar 3, 2025
8 checks passed
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