# HG changeset patch # User brutisso # Date 1367868634 -7200 # Node ID b0d20fa374b4dc0c15d1fb1d84075071a11ed152 # Parent d17700c82d7d52e8c84f03bd9f15ef2f918d5ae0 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 diff -r d17700c82d7d -r b0d20fa374b4 src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp --- a/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp Mon May 06 17:19:42 2013 +0200 +++ b/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()); diff -r d17700c82d7d -r b0d20fa374b4 src/share/vm/gc_implementation/g1/heapRegionSeq.cpp --- a/src/share/vm/gc_implementation/g1/heapRegionSeq.cpp Mon May 06 17:19:42 2013 +0200 +++ b/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 diff -r d17700c82d7d -r b0d20fa374b4 src/share/vm/gc_implementation/g1/heapRegionSeq.hpp --- a/src/share/vm/gc_implementation/g1/heapRegionSeq.hpp Mon May 06 17:19:42 2013 +0200 +++ b/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; diff -r d17700c82d7d -r b0d20fa374b4 test/gc/g1/TestShrinkToOneRegion.java --- /dev/null Thu Jan 01 00:00:00 1970 +0000 +++ b/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(); + } +}