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
8254739: G1: Optimize evacuation failure for regions with few failed objects #5181
Changes from 19 commits
954b5ee
4d88f42
74bc4d9
05f026a
ded8275
51d19eb
f454925
34aed3f
43a8b59
6132372
44e3562
e779c3a
924fec5
0070826
c4ca77c
ab04f1c
3712037
82c172a
d33f87b
e588cad
3efa90b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,131 @@ | ||
/* | ||
* Copyright (c) 2021, Huawei 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. | ||
* | ||
*/ | ||
|
||
#include "precompiled.hpp" | ||
#include "gc/g1/g1EvacFailureObjectsSet.hpp" | ||
#include "gc/g1/g1CollectedHeap.hpp" | ||
#include "gc/g1/g1SegmentedArray.inline.hpp" | ||
#include "gc/g1/heapRegion.hpp" | ||
#include "gc/g1/heapRegion.inline.hpp" | ||
#include "utilities/quickSort.hpp" | ||
|
||
|
||
const G1SegmentedArrayAllocOptions G1EvacFailureObjectsSet::_alloc_options = | ||
G1SegmentedArrayAllocOptions((uint)sizeof(OffsetInRegion), BufferLength, UINT_MAX, Alignment); | ||
|
||
G1SegmentedArrayBufferList<mtGC> G1EvacFailureObjectsSet::_free_buffer_list; | ||
|
||
#ifdef ASSERT | ||
void G1EvacFailureObjectsSet::assert_is_valid_offset(size_t offset) const { | ||
const uint max_offset = 1u << (HeapRegion::LogOfHRGrainBytes - LogHeapWordSize); | ||
assert(offset < max_offset, "must be, but is " SIZE_FORMAT, offset); | ||
} | ||
#endif | ||
|
||
oop G1EvacFailureObjectsSet::from_offset(OffsetInRegion offset) const { | ||
assert_is_valid_offset(offset); | ||
return cast_to_oop(_bottom + offset); | ||
} | ||
|
||
G1EvacFailureObjectsSet::OffsetInRegion G1EvacFailureObjectsSet::to_offset(oop obj) const { | ||
const HeapWord* o = cast_from_oop<const HeapWord*>(obj); | ||
size_t offset = pointer_delta(o, _bottom); | ||
assert(obj == from_offset(static_cast<OffsetInRegion>(offset)), "must be"); | ||
return static_cast<OffsetInRegion>(offset); | ||
} | ||
|
||
G1EvacFailureObjectsSet::G1EvacFailureObjectsSet(uint region_idx, HeapWord* bottom) : | ||
DEBUG_ONLY(_region_idx(region_idx) COMMA) | ||
_bottom(bottom), | ||
_offsets(&_alloc_options, &_free_buffer_list) { | ||
assert(HeapRegion::LogOfHRGrainBytes < 32, "must be"); | ||
} | ||
|
||
void G1EvacFailureObjectsSet::record(oop obj) { | ||
assert(obj != NULL, "must be"); | ||
assert(_region_idx == G1CollectedHeap::heap()->heap_region_containing(obj)->hrm_index(), "must be"); | ||
OffsetInRegion* e = _offsets.allocate(); | ||
*e = to_offset(obj); | ||
} | ||
|
||
// Helper class to join, sort and iterate over the previously collected segmented | ||
// array of objects that failed evacuation. | ||
class G1EvacFailureObjectsIterationHelper { | ||
typedef G1EvacFailureObjectsSet::OffsetInRegion OffsetInRegion; | ||
|
||
G1EvacFailureObjectsSet* _objects_set; | ||
const G1SegmentedArray<OffsetInRegion, mtGC>* _segments; | ||
OffsetInRegion* _offset_array; | ||
uint _array_length; | ||
|
||
static int order_oop(OffsetInRegion a, OffsetInRegion b) { | ||
return static_cast<int>(a-b); | ||
} | ||
|
||
void join_and_sort() { | ||
uint num = _segments->num_allocated_nodes(); | ||
_offset_array = NEW_C_HEAP_ARRAY(OffsetInRegion, num, mtGC); | ||
|
||
_segments->iterate_nodes(*this); | ||
assert(_array_length == num, "must be %u, %u", _array_length, num); | ||
|
||
QuickSort::sort(_offset_array, _array_length, order_oop, true); | ||
} | ||
|
||
void iterate_internal(ObjectClosure* closure) { | ||
for (uint i = 0; i < _array_length; i++) { | ||
oop cur = _objects_set->from_offset(_offset_array[i]); | ||
closure->do_object(cur); | ||
} | ||
|
||
FREE_C_HEAP_ARRAY(OffsetInRegion, _offset_array); | ||
} | ||
|
||
public: | ||
G1EvacFailureObjectsIterationHelper(G1EvacFailureObjectsSet* collector) : | ||
_objects_set(collector), | ||
_segments(&_objects_set->_offsets), | ||
_offset_array(nullptr), | ||
_array_length(0) { } | ||
|
||
void iterate(ObjectClosure* closure) { | ||
join_and_sort(); | ||
iterate_internal(closure); | ||
} | ||
|
||
// Callback of G1SegmentedArray::iterate_nodes | ||
void do_buffer(G1SegmentedArrayBuffer<mtGC>* node, uint length) { | ||
node->copy_to(&_offset_array[_array_length]); | ||
_array_length += length; | ||
} | ||
}; | ||
|
||
void G1EvacFailureObjectsSet::iterate(ObjectClosure* closure) { | ||
assert_at_safepoint(); | ||
|
||
G1EvacFailureObjectsIterationHelper helper(this); | ||
helper.iterate(closure); | ||
|
||
_offsets.drop_all(); | ||
} | ||
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. Having some destructive operations (
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. drop_all just returns buffers to free list, it will not destruct the buffers. So, iterate multiple times is OK, because next time it will get memory from free list or allocate a new buffer. Hope this answer your question. 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. Since 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. For example, we modify the code as below:
For the second time iteration, all thing will be empty, so iterate_nodes will be an empty operation, QuickSort::sort too, and iterate_internal too. These empty operations will not do harm things. 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. @Hamlin-Li : I think @albertnetymk concern is that typically an Maybe change I think such a change would solve Albert's concerns. An alternative could be renaming 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. Thank Thomas for unpacking my concern more precisely. I used "problematic" to mean the second iteration will not do what developers expect it to do, not necessarily a VM crash. 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. Thanks for your clarification, Albert, Thomas, I see your point, it make sense to me. As this patch has been blocking some other issues for a while, and I think it's better to think of some good solution for Albert's concern (seems add a drop is a little bit redundant for me :).) If you don't mind, can I do this refinement later in another issue? Thanks 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. It's a rather minor issue as I stated originally; I am fine either way. 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. Thanks, I created JDK-8276721 to track it. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,81 @@ | ||
/* | ||
* Copyright (c) 2021, Huawei 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. | ||
* | ||
*/ | ||
|
||
#ifndef SHARE_GC_G1_G1EVACUATIONFAILUREOBJSINHR_HPP | ||
#define SHARE_GC_G1_G1EVACUATIONFAILUREOBJSINHR_HPP | ||
|
||
#include "gc/g1/g1SegmentedArray.hpp" | ||
#include "memory/iterator.hpp" | ||
#include "oops/oop.hpp" | ||
|
||
class G1EvacFailureObjectsIterationHelper; | ||
|
||
// This class collects addresses of objects that failed evacuation in a specific | ||
// heap region. | ||
// Provides sorted iteration of these elements for processing during the remove | ||
// self forwards phase. | ||
class G1EvacFailureObjectsSet { | ||
friend class G1EvacFailureObjectsIterationHelper; | ||
|
||
public: | ||
// Storage type of an object that failed evacuation within a region. Given | ||
// heap region size and possible object locations within a region, it is | ||
// sufficient to use an uint here to save some space instead of full pointers. | ||
typedef uint OffsetInRegion; | ||
|
||
private: | ||
static const uint BufferLength = 256; | ||
static const uint Alignment = 4; | ||
|
||
static const G1SegmentedArrayAllocOptions _alloc_options; | ||
|
||
// This free list is shared among evacuation failure process in all regions. | ||
static G1SegmentedArrayBufferList<mtGC> _free_buffer_list; | ||
|
||
DEBUG_ONLY(const uint _region_idx;) | ||
|
||
// Region bottom | ||
const HeapWord* _bottom; | ||
|
||
// Offsets within region containing objects that failed evacuation. | ||
G1SegmentedArray<OffsetInRegion, mtGC> _offsets; | ||
|
||
void assert_is_valid_offset(size_t offset) const NOT_DEBUG_RETURN; | ||
// Converts between an offset within a region and an oop address. | ||
oop from_offset(OffsetInRegion offset) const; | ||
OffsetInRegion to_offset(oop obj) const; | ||
|
||
public: | ||
G1EvacFailureObjectsSet(uint region_idx, HeapWord* bottom); | ||
|
||
// Record an object that failed evacuation. | ||
void record(oop obj); | ||
|
||
// Apply the given ObjectClosure to all objects that failed evacuation. Objects | ||
// are passed in increasing address order. | ||
void iterate(ObjectClosure* closure); | ||
}; | ||
|
||
|
||
#endif //SHARE_GC_G1_G1EVACUATIONFAILUREOBJSINHR_HPP |
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 would probably move the array allocation and freeing here instead of having this at the start and end of
join_and_sort
anditerate_internal
respectively. Otherwise there is a hidden dependency on the first method allocating and the second freeing it, and looks cleaner as allocation and deallocation is obvious and on the same call level.I.e.
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.
Thanks Thomas, good catch.