truffle: src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp comparison

comparison src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp @ 4727:67fdcb391461

7119027: G1: use atomics to update RS length / predict time of inc CSet Summary: Make sure that the updates to the RS length and inc CSet predicted time are updated in an MT-safe way. Reviewed-by: brutisso, iveresov

author	tonyp
date	Wed, 21 Dec 2011 07:53:53 -0500
parents	41406797186b
children	441e946dc1af

comparison

equal deleted inserted replaced

-:d15b458c4225
+:67fdcb391461
 _inc_cset_head(NULL),
 _inc_cset_tail(NULL),
 _inc_cset_bytes_used_before(0),
 _inc_cset_max_finger(NULL),
 _inc_cset_recorded_rs_lengths(0),
+_inc_cset_recorded_rs_lengths_diffs(0),
 _inc_cset_predicted_elapsed_time_ms(0.0),
+_inc_cset_predicted_elapsed_time_ms_diffs(0.0),
 #ifdef _MSC_VER // the use of 'this' below gets a warning, make it go away
 #pragma warning( disable:4355 ) // 'this' : used in base member initializer list
 #endif // _MSC_VER
 } else {
 _mixed_cards_per_entry_ratio_seq->add(cards_per_entry_ratio);
 }
 }
-// It turns out that, sometimes, _max_rs_lengths can get smaller
+// This is defensive. For a while _max_rs_lengths could get
-// than _recorded_rs_lengths which causes rs_length_diff to get
+// smaller than _recorded_rs_lengths which was causing
-// very large and mess up the RSet length predictions. We'll be
+// rs_length_diff to get very large and mess up the RSet length
-// defensive until we work out why this happens.
+// predictions. The reason was unsafe concurrent updates to the
+// _inc_cset_recorded_rs_lengths field which the code below guards
+// against (see CR 7118202). This bug has now been fixed (see CR
+// 7119027). However, I'm still worried that
+// _inc_cset_recorded_rs_lengths might still end up somewhat
+// inaccurate. The concurrent refinement thread calculates an
+// RSet's length concurrently with other CR threads updating it
+// which might cause it to calculate the length incorrectly (if,
+// say, it's in mid-coarsening). So I'll leave in the defensive
+// conditional below just in case.
 size_t rs_length_diff = 0;
 if (_max_rs_lengths > _recorded_rs_lengths) {
 rs_length_diff = _max_rs_lengths - _recorded_rs_lengths;
 }
 _rs_length_diff_seq->add((double) rs_length_diff);
 _inc_cset_tail = NULL;
 _inc_cset_bytes_used_before = 0;
 _inc_cset_max_finger = 0;
 _inc_cset_recorded_rs_lengths = 0;
-_inc_cset_predicted_elapsed_time_ms = 0;
+_inc_cset_recorded_rs_lengths_diffs = 0;
+_inc_cset_predicted_elapsed_time_ms = 0.0;
+_inc_cset_predicted_elapsed_time_ms_diffs = 0.0;
 _inc_cset_build_state = Active;
+}
+void G1CollectorPolicy::finalize_incremental_cset_building() {
+assert(_inc_cset_build_state == Active, "Precondition");
+assert(SafepointSynchronize::is_at_safepoint(), "should be at a safepoint");
+// The two "main" fields, _inc_cset_recorded_rs_lengths and
+// _inc_cset_predicted_elapsed_time_ms, are updated by the thread
+// that adds a new region to the CSet. Further updates by the
+// concurrent refinement thread that samples the young RSet lengths
+// are accumulated in the *_diffs fields. Here we add the diffs to
+// the "main" fields.
+if (_inc_cset_recorded_rs_lengths_diffs >= 0) {
+_inc_cset_recorded_rs_lengths += _inc_cset_recorded_rs_lengths_diffs;
+} else {
+// This is defensive. The diff should in theory be always positive
+// as RSets can only grow between GCs. However, given that we
+// sample their size concurrently with other threads updating them
+// it's possible that we might get the wrong size back, which
+// could make the calculations somewhat inaccurate.
+size_t diffs = (size_t) (-_inc_cset_recorded_rs_lengths_diffs);
+if (_inc_cset_recorded_rs_lengths >= diffs) {
+_inc_cset_recorded_rs_lengths -= diffs;
+} else {
+_inc_cset_recorded_rs_lengths = 0;
+}
+}
+_inc_cset_predicted_elapsed_time_ms +=
+_inc_cset_predicted_elapsed_time_ms_diffs;
+_inc_cset_recorded_rs_lengths_diffs = 0;
+_inc_cset_predicted_elapsed_time_ms_diffs = 0.0;
 }
 void G1CollectorPolicy::add_to_incremental_cset_info(HeapRegion* hr, size_t rs_length) {
 // This routine is used when:
 // * adding survivor regions to the incremental cset at the end of an
 // retiring the current allocation region) or a concurrent
 // refine thread (RSet sampling).
 double region_elapsed_time_ms = predict_region_elapsed_time_ms(hr, true);
 size_t used_bytes = hr->used();
 _inc_cset_recorded_rs_lengths += rs_length;
 _inc_cset_predicted_elapsed_time_ms += region_elapsed_time_ms;
 _inc_cset_bytes_used_before += used_bytes;
 // Cache the values we have added to the aggregated informtion
 // in the heap region in case we have to remove this region from
 // the incremental collection set, or it is updated by the
 // rset sampling code
 hr->set_recorded_rs_length(rs_length);
 hr->set_predicted_elapsed_time_ms(region_elapsed_time_ms);
 }
-void G1CollectorPolicy::remove_from_incremental_cset_info(HeapRegion* hr) {
+void G1CollectorPolicy::update_incremental_cset_info(HeapRegion* hr,
-// This routine is currently only called as part of the updating of
+size_t new_rs_length) {
-// existing policy information for regions in the incremental cset that
+// Update the CSet information that is dependent on the new RS length
-// is performed by the concurrent refine thread(s) as part of young list
+assert(hr->is_young(), "Precondition");
-// RSet sampling. Therefore we should not be at a safepoint.
+assert(!SafepointSynchronize::is_at_safepoint(),
+"should not be at a safepoint");
-assert(!SafepointSynchronize::is_at_safepoint(), "should not be at safepoint");
-assert(hr->is_young(), "it should be");
+// We could have updated _inc_cset_recorded_rs_lengths and
+// _inc_cset_predicted_elapsed_time_ms directly but we'd need to do
-size_t used_bytes = hr->used();
+// that atomically, as this code is executed by a concurrent
-size_t old_rs_length = hr->recorded_rs_length();
+// refinement thread, potentially concurrently with a mutator thread
+// allocating a new region and also updating the same fields. To
+// avoid the atomic operations we accumulate these updates on two
+// separate fields (*_diffs) and we'll just add them to the "main"
+// fields at the start of a GC.
+ssize_t old_rs_length = (ssize_t) hr->recorded_rs_length();
+ssize_t rs_lengths_diff = (ssize_t) new_rs_length - old_rs_length;
+_inc_cset_recorded_rs_lengths_diffs += rs_lengths_diff;
 double old_elapsed_time_ms = hr->predicted_elapsed_time_ms();
+double new_region_elapsed_time_ms = predict_region_elapsed_time_ms(hr, true);
-// Subtract the old recorded/predicted policy information for
+double elapsed_ms_diff = new_region_elapsed_time_ms - old_elapsed_time_ms;
-// the given heap region from the collection set info.
+_inc_cset_predicted_elapsed_time_ms_diffs += elapsed_ms_diff;
-_inc_cset_recorded_rs_lengths -= old_rs_length;
-_inc_cset_predicted_elapsed_time_ms -= old_elapsed_time_ms;
+hr->set_recorded_rs_length(new_rs_length);
+hr->set_predicted_elapsed_time_ms(new_region_elapsed_time_ms);
-_inc_cset_bytes_used_before -= used_bytes;
-// Clear the values cached in the heap region
-hr->set_recorded_rs_length(0);
-hr->set_predicted_elapsed_time_ms(0);
-}
-void G1CollectorPolicy::update_incremental_cset_info(HeapRegion* hr, size_t new_rs_length) {
-// Update the collection set information that is dependent on the new RS length
-assert(hr->is_young(), "Precondition");
-remove_from_incremental_cset_info(hr);
-add_to_incremental_cset_info(hr, new_rs_length);
 }
 void G1CollectorPolicy::add_region_to_incremental_cset_common(HeapRegion* hr) {
 assert(hr->is_young(), "invariant");
 assert(hr->young_index_in_cset() > -1, "should have already been set");
 void G1CollectorPolicy::choose_collection_set(double target_pause_time_ms) {
 // Set this here - in case we're not doing young collections.
 double non_young_start_time_sec = os::elapsedTime();
 YoungList* young_list = _g1->young_list();
+finalize_incremental_cset_building();
 guarantee(target_pause_time_ms > 0.0,
 err_msg("target_pause_time_ms = %1.6lf should be positive",
 target_pause_time_ms));
 guarantee(_collection_set == NULL, "Precondition");

Mercurial > hg > truffle

comparison src/share/vm/gc_implementation/g1/g1CollectorPolicy.cpp @ 4727:67fdcb391461