8013872: G1: HeapRegionSeq::shrink_by() has invalid assert
Summary: Refactored shrink_by() to only use region counts and not byte sizes
Reviewed-by: johnc, tschatzl
--- a/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Mon May 06 17:19:42 2013 +0200
+++ b/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Mon May 06 21:30:34 2013 +0200
@@ -1843,33 +1843,32 @@
ReservedSpace::page_align_size_down(shrink_bytes);
aligned_shrink_bytes = align_size_down(aligned_shrink_bytes,
HeapRegion::GrainBytes);
- uint num_regions_deleted = 0;
- MemRegion mr = _hrs.shrink_by(aligned_shrink_bytes, &num_regions_deleted);
+ uint num_regions_to_remove = (uint)(shrink_bytes / HeapRegion::GrainBytes);
+
+ uint num_regions_removed = _hrs.shrink_by(num_regions_to_remove);
HeapWord* old_end = (HeapWord*) _g1_storage.high();
- assert(mr.end() == old_end, "post-condition");
+ size_t shrunk_bytes = num_regions_removed * HeapRegion::GrainBytes;
ergo_verbose3(ErgoHeapSizing,
"shrink the heap",
ergo_format_byte("requested shrinking amount")
ergo_format_byte("aligned shrinking amount")
ergo_format_byte("attempted shrinking amount"),
- shrink_bytes, aligned_shrink_bytes, mr.byte_size());
- if (mr.byte_size() > 0) {
+ shrink_bytes, aligned_shrink_bytes, shrunk_bytes);
+ if (num_regions_removed > 0) {
+ _g1_storage.shrink_by(shrunk_bytes);
+ HeapWord* new_end = (HeapWord*) _g1_storage.high();
+
if (_hr_printer.is_active()) {
- HeapWord* curr = mr.end();
- while (curr > mr.start()) {
+ HeapWord* curr = old_end;
+ while (curr > new_end) {
HeapWord* curr_end = curr;
curr -= HeapRegion::GrainWords;
_hr_printer.uncommit(curr, curr_end);
}
- assert(curr == mr.start(), "post-condition");
}
- _g1_storage.shrink_by(mr.byte_size());
- HeapWord* new_end = (HeapWord*) _g1_storage.high();
- assert(mr.start() == new_end, "post-condition");
-
- _expansion_regions += num_regions_deleted;
+ _expansion_regions += num_regions_removed;
update_committed_space(old_end, new_end);
HeapRegionRemSet::shrink_heap(n_regions());
g1_policy()->record_new_heap_size(n_regions());
--- a/hotspot/src/share/vm/gc_implementation/g1/heapRegionSeq.cpp Mon May 06 17:19:42 2013 +0200
+++ b/hotspot/src/share/vm/gc_implementation/g1/heapRegionSeq.cpp Mon May 06 21:30:34 2013 +0200
@@ -124,11 +124,11 @@
}
assert(_regions[index] == NULL, "invariant");
_regions[index] = new_hr;
- increment_length(&_allocated_length);
+ increment_allocated_length();
}
// Have to increment the length first, otherwise we will get an
// assert failure at(index) below.
- increment_length(&_length);
+ increment_length();
HeapRegion* hr = at(index);
list->add_as_tail(hr);
@@ -201,45 +201,29 @@
}
}
-MemRegion HeapRegionSeq::shrink_by(size_t shrink_bytes,
- uint* num_regions_deleted) {
+uint HeapRegionSeq::shrink_by(uint num_regions_to_remove) {
// Reset this in case it's currently pointing into the regions that
// we just removed.
_next_search_index = 0;
- assert(shrink_bytes % os::vm_page_size() == 0, "unaligned");
- assert(shrink_bytes % HeapRegion::GrainBytes == 0, "unaligned");
assert(length() > 0, "the region sequence should not be empty");
assert(length() <= _allocated_length, "invariant");
assert(_allocated_length > 0, "we should have at least one region committed");
+ assert(num_regions_to_remove < length(), "We should never remove all regions");
- // around the loop, i will be the next region to be removed
- uint i = length() - 1;
- assert(i > 0, "we should never remove all regions");
- // [last_start, end) is the MemRegion that covers the regions we will remove.
- HeapWord* end = at(i)->end();
- HeapWord* last_start = end;
- *num_regions_deleted = 0;
- while (shrink_bytes > 0) {
- HeapRegion* cur = at(i);
- // We should leave the humongous regions where they are.
- if (cur->isHumongous()) break;
- // We should stop shrinking if we come across a non-empty region.
- if (!cur->is_empty()) break;
+ uint i = 0;
+ for (; i < num_regions_to_remove; i++) {
+ HeapRegion* cur = at(length() - 1);
- i -= 1;
- *num_regions_deleted += 1;
- shrink_bytes -= cur->capacity();
- last_start = cur->bottom();
- decrement_length(&_length);
- // We will reclaim the HeapRegion. _allocated_length should be
- // covering this index. So, even though we removed the region from
- // the active set by decreasing _length, we still have it
- // available in the future if we need to re-use it.
- assert(i > 0, "we should never remove all regions");
- assert(length() > 0, "we should never remove all regions");
+ if (!cur->is_empty()) {
+ // We have to give up if the region can not be moved
+ break;
}
- return MemRegion(last_start, end);
+ assert(!cur->isHumongous(), "Humongous regions should not be empty");
+
+ decrement_length();
+ }
+ return i;
}
#ifndef PRODUCT
--- a/hotspot/src/share/vm/gc_implementation/g1/heapRegionSeq.hpp Mon May 06 17:19:42 2013 +0200
+++ b/hotspot/src/share/vm/gc_implementation/g1/heapRegionSeq.hpp Mon May 06 21:30:34 2013 +0200
@@ -92,14 +92,19 @@
// address is valid.
inline uintx addr_to_index_biased(HeapWord* addr) const;
- void increment_length(uint* length) {
- assert(*length < _max_length, "pre-condition");
- *length += 1;
+ void increment_allocated_length() {
+ assert(_allocated_length < _max_length, "pre-condition");
+ _allocated_length++;
}
- void decrement_length(uint* length) {
- assert(*length > 0, "pre-condition");
- *length -= 1;
+ void increment_length() {
+ assert(_length < _max_length, "pre-condition");
+ _length++;
+ }
+
+ void decrement_length() {
+ assert(_length > 0, "pre-condition");
+ _length--;
}
public:
@@ -153,11 +158,9 @@
void iterate_from(HeapRegion* hr, HeapRegionClosure* blk) const;
// Tag as uncommitted as many regions that are completely free as
- // possible, up to shrink_bytes, from the suffix of the committed
- // sequence. Return a MemRegion that corresponds to the address
- // range of the uncommitted regions. Assume shrink_bytes is page and
- // heap region aligned.
- MemRegion shrink_by(size_t shrink_bytes, uint* num_regions_deleted);
+ // possible, up to num_regions_to_remove, from the suffix of the committed
+ // sequence. Return the actual number of removed regions.
+ uint shrink_by(uint num_regions_to_remove);
// Do some sanity checking.
void verify_optional() PRODUCT_RETURN;
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/hotspot/test/gc/g1/TestShrinkToOneRegion.java Mon May 06 21:30:34 2013 +0200
@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2013, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test TestShrinkToOneRegion.java
+ * @bug 8013872
+ * @summary Shrinking the heap down to one region used to hit an assert
+ * @run main/othervm -XX:+UseG1GC -XX:G1HeapRegionSize=32m -Xmx256m TestShrinkToOneRegion
+ *
+ * Doing a System.gc() without having allocated many objects will shrink the heap.
+ * With a large region size we will shrink the heap to one region.
+ */
+public class TestShrinkToOneRegion {
+ public static void main(String[] args) {
+ System.gc();
+ }
+}