8143041: Unify G1CollectorPolicy::PauseKind and G1YCType #2870
Conversation
|
👋 Welcome back iwalulya! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
|
||
| #include "utilities/debug.hpp" | ||
|
|
||
| enum G1YCType { | ||
| enum G1YCPhase { |
There was a problem hiding this comment.
I kind of think a better name would be G1YoungPhase - the C in that name is an abbreviation for "collection", and to me, YCPhase indicates the phases in the collection which this is not.
Better names are certainly possible.
Please also add some small documentation, like "The phases the collection cycle can be in".
| }; | ||
|
|
||
| class G1YCTypeHelper { | ||
| enum G1GCType { |
There was a problem hiding this comment.
Not sure if it might be better to name this G1PauseType since Cleanup and Remark aren't really collections. But maybe it's fine as is (or G1GCPauseType if you prefer).
| @@ -46,6 +84,7 @@ class G1YCTypeHelper { | |||
| default: ShouldNotReachHere(); return NULL; | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Extra newline that can be deleted.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| * Copyright (c) 2012, 2019, Oracle and/or its affiliates. All rights reserved. | |||
| * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved. | |||
There was a problem hiding this comment.
This should probably be 2012, 2021, - I do not see a reason to remove the former date.
There was a problem hiding this comment.
webrev shows this as a new file, while github shows it as a renamed file. Not sure which one to consider.
| static const char* to_string(G1YCType type) { | ||
|
|
||
| static bool is_young_only_pause(G1GCType type) { | ||
| assert(type != FullGC, "must be"); |
There was a problem hiding this comment.
These three asserts could probably be moved into a helper method as the exact same code is used in is_mixed_pause() too. Your call.
| public: | ||
| static const char* to_string(G1YCType type) { | ||
|
|
||
| static bool is_young_only_pause(G1GCType type) { |
There was a problem hiding this comment.
is_young_pause_young_only()? Just to match the method. Feel free to ignore.
There was a problem hiding this comment.
I think adding the assert_is_young_pause should remove the need to add is_young_pause prefix to these methods.
| type == YoungGC; | ||
| } | ||
|
|
||
| static bool is_mixed_pause(G1GCType type) { |
There was a problem hiding this comment.
is_young_pause_mixed()? Just to match the method. Feel free to ignore.
| return type == MixedGC; | ||
| } | ||
|
|
||
| static bool is_last_young_pause(G1GCType type) { |
There was a problem hiding this comment.
is_young_pause_last_young(). I also think this method is missing the three asserts of the other methods (pre-existing).
| return type == LastYoungGC; | ||
| } | ||
|
|
||
| static bool is_concurrent_start_pause(G1GCType type) { |
There was a problem hiding this comment.
is_young_pause_concurrent_start(). Same as above, it should have the three asserts.
|
Thanks @tschatzl , I have updated with the proposed changes |
|
@walulyai This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 45 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
Thanks @tschatzl and @albertnetymk for the reviews |
|
/integrate |
openjdk
bot
added
integrated
|
@walulyai Since your change was applied there have been 54 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 470b150. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this refactoring change to unify G1CollectorPolicy::PauseKind and G1YCType enums under the same file. Additionally, make the distinction between collector types and phases clearer.
Progress
Issue
Reviewers
Download
$ git fetch https://git.openjdk.java.net/jdk pull/2870/head:pull/2870$ git checkout pull/2870