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

8228609: G1 copy cost prediction uses used vs. actual copied bytes #927

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/hotspot/share/gc/g1/g1CollectedHeap.cpp
Expand Up @@ -2883,7 +2883,6 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) {
collector_state()->yc_type() == Mixed /* allMemoryPoolsAffected */);

G1HeapTransition heap_transition(this);
size_t heap_used_bytes_before_gc = used();

// Don't dynamically change the number of GC threads this early. A value of
// 0 is used to indicate serial work. When parallel work is done,
Expand Down Expand Up @@ -3044,7 +3043,7 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) {
double sample_end_time_sec = os::elapsedTime();
double pause_time_ms = (sample_end_time_sec - sample_start_time_sec) * MILLIUNITS;
size_t total_cards_scanned = g1_policy()->phase_times()->sum_thread_work_items(G1GCPhaseTimes::ScanRS, G1GCPhaseTimes::ScanRSScannedCards);
g1_policy()->record_collection_pause_end(pause_time_ms, total_cards_scanned, heap_used_bytes_before_gc);
g1_policy()->record_collection_pause_end(pause_time_ms, total_cards_scanned);

evacuation_info.set_collectionset_used_before(collection_set()->bytes_used_before());
evacuation_info.set_bytes_copied(g1_policy()->bytes_copied_during_gc());
Expand Down
11 changes: 8 additions & 3 deletions src/hotspot/share/gc/g1/g1ParScanThreadState.cpp
Expand Up @@ -82,16 +82,19 @@ G1ParScanThreadState::G1ParScanThreadState(G1CollectedHeap* g1h, uint worker_id,
}

// Pass locally gathered statistics to global state.
void G1ParScanThreadState::flush(size_t* surviving_young_words) {
size_t G1ParScanThreadState::flush(size_t* surviving_young_words) {
_dcq.flush();
// Update allocation statistics.
_plab_allocator->flush_and_retire_stats();
_g1h->g1_policy()->record_age_table(&_age_table);

size_t sum = 0;
uint length = _g1h->collection_set()->young_region_length();
for (uint region_index = 0; region_index < length; region_index++) {
surviving_young_words[region_index] += _surviving_young_words[region_index];
surviving_young_words[region_index] += G1ParScanThreadState::surviving_young_words()[region_index];
Copy link
Contributor

Choose a reason for hiding this comment

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

Was that code using the wrong index before? IOW, it used the surviving_young_words of the wrong regions?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that's what I think.

So this pr is achieving two things (sorry if it were not clear):

  1. fix the index issue: this causes calculation of the total surviving young words to be incorrect. The correct stats are stored in _surviving_young_words[1, .., n] instead of [0, .., n-1]
  2. use a precise "copied bytes" instead of inferring from (used() - freed())

sum += G1ParScanThreadState::surviving_young_words()[region_index];
}
return sum;
}

G1ParScanThreadState::~G1ParScanThreadState() {
Expand Down Expand Up @@ -339,17 +342,19 @@ const size_t* G1ParScanThreadStateSet::surviving_young_words() const {
void G1ParScanThreadStateSet::flush() {
assert(!_flushed, "thread local state from the per thread states should be flushed once");

size_t copied_words = 0;
for (uint worker_index = 0; worker_index < _n_workers; ++worker_index) {
G1ParScanThreadState* pss = _states[worker_index];

if (pss == NULL) {
continue;
}

pss->flush(_surviving_young_words_total);
copied_words += pss->flush(_surviving_young_words_total);
delete pss;
_states[worker_index] = NULL;
}
_g1h->g1_policy()->record_copied_bytes(copied_words << LogBytesPerWord);
_flushed = true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/gc/g1/g1ParScanThreadState.hpp
Expand Up @@ -132,7 +132,7 @@ class G1ParScanThreadState : public CHeapObj<mtGC> {
return _surviving_young_words + 1;
}

void flush(size_t* surviving_young_words);
size_t flush(size_t* surviving_young_words);

private:
#define G1_PARTIAL_ARRAY_MASK 0x2
Expand Down
12 changes: 4 additions & 8 deletions src/hotspot/share/gc/g1/g1Policy.cpp
Expand Up @@ -60,6 +60,7 @@ G1Policy::G1Policy(STWGCTimer* gc_timer) :
_reserve_factor((double) G1ReservePercent / 100.0),
_reserve_regions(0),
_rs_lengths_prediction(0),
_copied_bytes(0),
_initial_mark_to_mixed(),
_collection_set(NULL),
_g1h(NULL),
Expand Down Expand Up @@ -553,11 +554,9 @@ bool G1Policy::need_to_start_conc_mark(const char* source, size_t alloc_word_siz
// Anything below that is considered to be zero
#define MIN_TIMER_GRANULARITY 0.0000001

void G1Policy::record_collection_pause_end(double pause_time_ms, size_t cards_scanned, size_t heap_used_bytes_before_gc) {
void G1Policy::record_collection_pause_end(double pause_time_ms, size_t cards_scanned) {
double end_time_sec = os::elapsedTime();

size_t cur_used_bytes = _g1h->used();
assert(cur_used_bytes == _g1h->recalculate_used(), "It should!");
bool this_pause_included_initial_mark = false;
bool this_pause_was_young_only = collector_state()->in_young_only_phase();

Expand Down Expand Up @@ -665,12 +664,9 @@ void G1Policy::record_collection_pause_end(double pause_time_ms, size_t cards_sc
}
_analytics->report_rs_length_diff((double) rs_length_diff);

size_t freed_bytes = heap_used_bytes_before_gc - cur_used_bytes;

if (_collection_set->bytes_used_before() > freed_bytes) {
size_t copied_bytes = _collection_set->bytes_used_before() - freed_bytes;
if (_copied_bytes > 0) {
double average_copy_time = average_time_ms(G1GCPhaseTimes::ObjCopy);
double cost_per_byte_ms = average_copy_time / (double) copied_bytes;
double cost_per_byte_ms = average_copy_time / (double) _copied_bytes;
_analytics->report_cost_per_byte_ms(cost_per_byte_ms, collector_state()->mark_or_rebuild_in_progress());
}

Expand Down
8 changes: 7 additions & 1 deletion src/hotspot/share/gc/g1/g1Policy.hpp
Expand Up @@ -106,6 +106,8 @@ class G1Policy: public CHeapObj<mtGC> {

size_t _pending_cards;

size_t _copied_bytes;

G1InitialMarkToMixedTimeTracker _initial_mark_to_mixed;

bool should_update_surv_rate_group_predictors() {
Expand Down Expand Up @@ -133,6 +135,10 @@ class G1Policy: public CHeapObj<mtGC> {
_max_rs_lengths = rs_lengths;
}

void record_copied_bytes(size_t copied_bytes) {
_copied_bytes = copied_bytes;
}

double predict_base_elapsed_time_ms(size_t pending_cards) const;
double predict_base_elapsed_time_ms(size_t pending_cards,
size_t scanned_cards) const;
Expand Down Expand Up @@ -308,7 +314,7 @@ class G1Policy: public CHeapObj<mtGC> {

// Record the start and end of an evacuation pause.
void record_collection_pause_start(double start_time_sec);
void record_collection_pause_end(double pause_time_ms, size_t cards_scanned, size_t heap_used_bytes_before_gc);
void record_collection_pause_end(double pause_time_ms, size_t cards_scanned);

// Record the start and end of a full collection.
void record_full_collection_start();
Expand Down