-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8270014: Add scoped objects for g1 young gc verification and young gc internal timing #4768
Closed
tschatzl
wants to merge
5
commits into
openjdk:master
from
tschatzl:submit/8270014-scope-objects-verification-3
Closed
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
c321d38
Initial implementation: further refactoring
tschatzl 7730aa6
Fix test because of slightly different log message order
tschatzl b9b53f8
ayang review
tschatzl b6be6fc
Change note_ to record_
tschatzl 86dfe71
kbarrett review
tschatzl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,8 +113,8 @@ void G1Policy::init(G1CollectedHeap* g1h, G1CollectionSet* collection_set) { | |
_collection_set->start_incremental_building(); | ||
} | ||
|
||
void G1Policy::note_gc_start() { | ||
phase_times()->note_gc_start(); | ||
void G1Policy::record_young_gc_pause_start() { | ||
phase_times()->record_gc_pause_start(); | ||
} | ||
|
||
class G1YoungLengthPredictor { | ||
|
@@ -517,7 +517,8 @@ void G1Policy::record_concurrent_refinement_stats() { | |
} | ||
} | ||
|
||
void G1Policy::record_collection_pause_start(double start_time_sec) { | ||
void G1Policy::record_young_collection_start() { | ||
Ticks now = Ticks::now(); | ||
// We only need to do this here as the policy will only be applied | ||
// to the GC we're about to start. so, no point is calculating this | ||
// every time we calculate / recalculate the target young length. | ||
|
@@ -528,7 +529,7 @@ void G1Policy::record_collection_pause_start(double start_time_sec) { | |
max_survivor_regions(), _g1h->num_used_regions(), _g1h->max_regions()); | ||
assert_used_and_recalculate_used_equal(_g1h); | ||
|
||
phase_times()->record_cur_collection_start_sec(start_time_sec); | ||
phase_times()->record_cur_collection_start_sec(now.seconds()); | ||
|
||
record_concurrent_refinement_stats(); | ||
|
||
|
@@ -628,11 +629,12 @@ double G1Policy::logged_cards_processing_time() const { | |
// Anything below that is considered to be zero | ||
#define MIN_TIMER_GRANULARITY 0.0000001 | ||
|
||
void G1Policy::record_collection_pause_end(double pause_time_ms, bool concurrent_operation_is_full_mark) { | ||
void G1Policy::record_young_collection_end(bool concurrent_operation_is_full_mark) { | ||
G1GCPhaseTimes* p = phase_times(); | ||
|
||
double end_time_sec = os::elapsedTime(); | ||
double start_time_sec = phase_times()->cur_collection_start_sec(); | ||
double start_time_s = phase_times()->cur_collection_start_sec(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dislike this change of nomenclature to use |
||
double end_time_s = Ticks::now().seconds(); | ||
double pause_time_ms = (end_time_s - start_time_s) * 1000.0; | ||
|
||
G1GCPauseType this_pause = collector_state()->young_gc_pause_type(concurrent_operation_is_full_mark); | ||
|
||
|
@@ -644,7 +646,7 @@ void G1Policy::record_collection_pause_end(double pause_time_ms, bool concurrent | |
maybe_start_marking(); | ||
} | ||
|
||
double app_time_ms = (start_time_sec * 1000.0 - _analytics->prev_collection_pause_end_ms()); | ||
double app_time_ms = (start_time_s * 1000.0 - _analytics->prev_collection_pause_end_ms()); | ||
if (app_time_ms < MIN_TIMER_GRANULARITY) { | ||
// This usually happens due to the timer not having the required | ||
// granularity. Some Linuxes are the usual culprits. | ||
|
@@ -666,7 +668,7 @@ void G1Policy::record_collection_pause_end(double pause_time_ms, bool concurrent | |
_analytics->report_alloc_rate_ms(alloc_rate_ms); | ||
} | ||
|
||
record_pause(this_pause, start_time_sec, end_time_sec); | ||
record_pause(this_pause, start_time_s, end_time_s); | ||
|
||
if (G1GCPauseTypeHelper::is_last_young_pause(this_pause)) { | ||
assert(!G1GCPauseTypeHelper::is_concurrent_start_pause(this_pause), | ||
|
@@ -889,7 +891,8 @@ void G1Policy::report_ihop_statistics() { | |
_ihop_control->print(); | ||
} | ||
|
||
void G1Policy::print_phases() { | ||
void G1Policy::record_young_gc_pause_end() { | ||
phase_times()->record_gc_pause_end(); | ||
phase_times()->print(); | ||
} | ||
|
||
|
@@ -1487,7 +1490,7 @@ void G1Policy::update_survival_estimates_for_next_collection() { | |
} | ||
|
||
void G1Policy::transfer_survivors_to_cset(const G1SurvivorRegions* survivors) { | ||
note_start_adding_survivor_regions(); | ||
start_adding_survivor_regions(); | ||
|
||
HeapRegion* last = NULL; | ||
for (GrowableArrayIterator<HeapRegion*> it = survivors->regions()->begin(); | ||
|
@@ -1503,7 +1506,7 @@ void G1Policy::transfer_survivors_to_cset(const G1SurvivorRegions* survivors) { | |
|
||
last = curr; | ||
} | ||
note_stop_adding_survivor_regions(); | ||
stop_adding_survivor_regions(); | ||
|
||
// Don't clear the survivor list handles until the start of | ||
// the next evacuation pause - we need it in order to re-tag | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this
if
just checking whether verification is enabled or not? If so, canVerifyBeforeGC
be used? Otherwise, it's unclear to me why we are comparing it with0.0
.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
if
checks whether any verification work has been done (it will be > 0.0 if it has been set) in this pause. We can't just useVerifyBefore/AfterGC
because verification before/after does not always run for all young gc pauses (seeVerifyGCType
).If we want to change this (existing) behavior to like always print the time if the flag is enabled I would suggest to file another CR instead of hiding it here (I can do all that and the associated work if that is desired).
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 see; thank you for the explanation.
Either that or just put some comment there, since in what situation the condition is true is not obvious. Ofc, this is very subjective.