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

8254739: G1: Optimize evacuation failure for regions with few failed objects #5181

Closed
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
954b5ee
speed up iterate evac failure objs in one region.
Hamlin-Li Aug 18, 2021
4d88f42
fix: reset state after iteration
Hamlin-Li Aug 18, 2021
74bc4d9
Use a segmented array rather than a linked list to record evacuation …
Hamlin-Li Aug 25, 2021
05f026a
Fix compilation issues on some platform.
Hamlin-Li Aug 25, 2021
ded8275
Fix test failures; Fix compilation failures on some platforms
Hamlin-Li Aug 26, 2021
51d19eb
Fix test failures; Fix compilation failures on some platforms
Hamlin-Li Aug 26, 2021
f454925
Fix compilation error on windows
Hamlin-Li Aug 26, 2021
34aed3f
Fix compilation error on windows
Hamlin-Li Aug 26, 2021
43a8b59
Merge branch 'master' into speedup-iterate-evac-failure-objs-in-one-r…
Hamlin-Li Oct 25, 2021
6132372
Fix wrong merge
Hamlin-Li Oct 25, 2021
44e3562
Use refactored G1SegmentedArray rather than home-made Array+Node
Hamlin-Li Oct 25, 2021
e779c3a
Add asserts, comments
Hamlin-Li Oct 26, 2021
924fec5
Rename from g1EvacuationFailureObjsInHR to g1EvacFailureObjsInHR
Hamlin-Li Oct 26, 2021
0070826
Refactor as Thomas suggested
Hamlin-Li Oct 29, 2021
c4ca77c
Fix compilation error
Hamlin-Li Oct 29, 2021
ab04f1c
Fix compilation error
Hamlin-Li Oct 29, 2021
3712037
Merge branch 'openjdk:master' into speedup-iterate-evac-failure-objs-…
Hamlin-Li Oct 29, 2021
82c172a
Refine code based on Thomas' suggestion
Hamlin-Li Oct 29, 2021
d33f87b
Merge branch 'openjdk:master' into speedup-iterate-evac-failure-objs-…
Hamlin-Li Nov 3, 2021
e588cad
Move allocation/deallocation in one place
Hamlin-Li Nov 3, 2021
3efa90b
Fix typo
Hamlin-Li Nov 5, 2021
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
@@ -25,6 +25,7 @@
#include "precompiled.hpp"

#include "gc/g1/g1CardSetMemory.inline.hpp"
#include "gc/g1/g1SegmentedArray.inline.hpp"
#include "logging/log.hpp"
#include "runtime/atomic.hpp"
#include "utilities/formatBuffer.hpp"
@@ -46,6 +47,11 @@ G1CardSetAllocator<Elem>::G1CardSetAllocator(const char* name,
assert(elem_size >= sizeof(G1CardSetContainer), "Element instance size %u for allocator %s too small", elem_size, name);
}

template <class Elem>
G1CardSetAllocator<Elem>::~G1CardSetAllocator() {
drop_all();
}

template <class Elem>
bool G1CardSetAllocator<Elem>::try_transfer_pending() {
// Attempt to claim the lock.
@@ -113,9 +113,7 @@ class G1CardSetAllocator {
G1CardSetAllocator(const char* name,
const G1CardSetAllocOptions* buffer_options,
G1CardSetBufferList* free_buffer_list);
~G1CardSetAllocator() {
drop_all();
}
~G1CardSetAllocator();

Elem* allocate();
void free(Elem* elem);
@@ -68,40 +68,40 @@ class RemoveSelfForwardPtrObjClosure: public ObjectClosure {
// dead too) already.
void do_object(oop obj) {
HeapWord* obj_addr = cast_from_oop<HeapWord*>(obj);
assert(_last_forwarded_object_end <= obj_addr, "should iterate in ascending address order");
assert(_hr->is_in(obj_addr), "sanity");

if (obj->is_forwarded() && obj->forwardee() == obj) {
// The object failed to move.
// The object failed to move.
assert(obj->is_forwarded() && obj->forwardee() == obj, "sanity");

zap_dead_objects(_last_forwarded_object_end, obj_addr);
// We consider all objects that we find self-forwarded to be
// live. What we'll do is that we'll update the prev marking
// info so that they are all under PTAMS and explicitly marked.
if (!_cm->is_marked_in_prev_bitmap(obj)) {
_cm->mark_in_prev_bitmap(obj);
}
if (_during_concurrent_start) {
// For the next marking info we'll only mark the
// self-forwarded objects explicitly if we are during
// concurrent start (since, normally, we only mark objects pointed
// to by roots if we succeed in copying them). By marking all
// self-forwarded objects we ensure that we mark any that are
// still pointed to be roots. During concurrent marking, and
// after concurrent start, we don't need to mark any objects
// explicitly and all objects in the CSet are considered
// (implicitly) live. So, we won't mark them explicitly and
// we'll leave them over NTAMS.
_cm->mark_in_next_bitmap(_worker_id, _hr, obj);
}
size_t obj_size = obj->size();
zap_dead_objects(_last_forwarded_object_end, obj_addr);
// We consider all objects that we find self-forwarded to be
// live. What we'll do is that we'll update the prev marking
// info so that they are all under PTAMS and explicitly marked.
if (!_cm->is_marked_in_prev_bitmap(obj)) {
_cm->mark_in_prev_bitmap(obj);
}
if (_during_concurrent_start) {
// For the next marking info we'll only mark the
// self-forwarded objects explicitly if we are during
// concurrent start (since, normally, we only mark objects pointed
// to by roots if we succeed in copying them). By marking all
// self-forwarded objects we ensure that we mark any that are
// still pointed to be roots. During concurrent marking, and
// after concurrent start, we don't need to mark any objects
// explicitly and all objects in the CSet are considered
// (implicitly) live. So, we won't mark them explicitly and
// we'll leave them over NTAMS.
_cm->mark_in_next_bitmap(_worker_id, _hr, obj);
}
size_t obj_size = obj->size();

_marked_bytes += (obj_size * HeapWordSize);
PreservedMarks::init_forwarded_mark(obj);
_marked_bytes += (obj_size * HeapWordSize);
PreservedMarks::init_forwarded_mark(obj);

HeapWord* obj_end = obj_addr + obj_size;
_last_forwarded_object_end = obj_end;
_hr->alloc_block_in_bot(obj_addr, obj_end);
}
HeapWord* obj_end = obj_addr + obj_size;
_last_forwarded_object_end = obj_end;
_hr->alloc_block_in_bot(obj_addr, obj_end);
}

// Fill the memory area from start to end with filler objects, and update the BOT
@@ -164,7 +164,8 @@ class RemoveSelfForwardPtrHRClosure: public HeapRegionClosure {
RemoveSelfForwardPtrObjClosure rspc(hr,
during_concurrent_start,
_worker_id);
hr->object_iterate(&rspc);
// Iterates evac failure objs which are recorded during evacuation.
hr->iterate_evac_failure_objs(&rspc);
// Need to zap the remainder area of the processed region.
rspc.zap_remainder();

@@ -0,0 +1,149 @@
/*
* 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::cast_to_offset(oop obj) const {
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

This should probably be to_offset to match from_offset (forgot that) - this is not a cast to me (changing the interpretation of a bit pattern) but a transformation (changing the bit pattern for storage savings).

So cast seems to be the wrong word to me here.

const HeapWord* o = cast_from_oop<const HeapWord*>(obj);
size_t offset = pointer_delta(o, _bottom);
assert_is_valid_offset(offset);
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

The from_offset call below already does this assert.

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) {
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

Fwiw, I recently removed the first parameter of G1SegmentedArray. Please make sure you merge with tip before pushing.

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 = cast_to_offset(obj);
}

// Helper class to join, sort and iterate over the previously collected segmented
// array of objects that failed evacuation.
class G1EvacFailureObjectsIterator {
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

Note that this is not an Iterator in C++ STL sense, so it is a good idea to not name it like that. I understand that Hotspot code (and even the recent remembered set code - there is a CR to fix that) adds to the confusion, but it would be nice to not add to the confusion.

typedef G1EvacFailureObjectsSet::OffsetInRegion OffsetInRegion;
friend class G1SegmentedArray<OffsetInRegion, mtGC>;
friend class G1SegmentedArrayBuffer<mtGC>;
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

I prefer to make visit_* public here instead of the friends. This is an internal class not visible outside, and we actually use it as a closure.


G1EvacFailureObjectsSet* _collector;
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

Probably rename _collector to _objects_set or so to match the type better.

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++) {
_collector->assert_is_valid_offset(_offset_array[i]);
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

This assert duplicates the one in from_offset below. Please remove.

oop cur = _collector->from_offset(_offset_array[i]);
closure->do_object(cur);
}

FREE_C_HEAP_ARRAY(OffsetInRegion, _offset_array);
}

// Callback of G1SegmentedArray::iterate_nodes
void visit_buffer(G1SegmentedArrayBuffer<mtGC>* node, uint length) {
node->copy_to(&_offset_array[_array_length]);
_array_length += length;

// Verify elements in the node
DEBUG_ONLY(node->iterate_elems(*this));
}

#ifdef ASSERT
// Callback of G1SegmentedArrayBuffer::iterate_elems
// Verify a single element in a segment node
void visit_elem(void* elem) {
uint* ptr = (uint*)elem;
_collector->assert_is_valid_offset(*ptr);
}
#endif
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

I intentionally removed this visit_elem method in the original suggestion because I thought that to be too much checking. It is kind of obvious that we only store data in here.
We check that these are valid offsets both when writing to and reading from the array, so this seems to be unnecessary triple-checking.

Fwiw, in Hotspot code we do not use the visit_ prefix but do_ (and pass something that ends with a Closure, not a Visitor in the iterate* methods).
I am aware that in design pattern lingo this is the Visitor pattern, but Hotspot is probably much older than that and it is somewhat jarring to have some code use this terminology and others use another one.

A change here needs to be discussed with a wider audience.


public:
G1EvacFailureObjectsIterator(G1EvacFailureObjectsSet* collector) :
_collector(collector),
_segments(&_collector->_offsets),
_offset_array(nullptr),
_array_length(0) { }

~G1EvacFailureObjectsIterator() { }
Copy link
Contributor

@tschatzl tschatzl Oct 29, 2021

Choose a reason for hiding this comment

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

Please remove the empty unnecessary destructor.


void iterate(ObjectClosure* closure) {
join_and_sort();
iterate_internal(closure);
Copy link
Contributor

@tschatzl tschatzl Nov 3, 2021

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 and iterate_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.

  void iterate(ObjectClosure* closure) {
     uint num = _segments->num_allocated_nodes();
    _offset_array = NEW_C_HEAP_ARRAY(OffsetInRegion, num, mtGC);

    join_and_sort();
    iterate_internal(closure);

    FREE_C_HEAP_ARRAY(OffsetInRegion, _offset_array);
  }

Copy link
Author

@Hamlin-Li Hamlin-Li Nov 3, 2021

Choose a reason for hiding this comment

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

Thanks Thomas, good catch.

}
};

void G1EvacFailureObjectsSet::iterate(ObjectClosure* closure) {
assert_at_safepoint();

G1EvacFailureObjectsIterator iterator(this);
iterator.iterate(closure);

_offsets.drop_all();
}
@@ -0,0 +1,82 @@
/*
* 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 G1EvacFailureObjectsIterator;

// 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 G1EvacFailureObjectsIterator;

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 cast_to_offset(oop obj) const;

public:
G1EvacFailureObjectsSet(uint region_idx, HeapWord* bottom);
~G1EvacFailureObjectsSet() { }

// 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
@@ -607,6 +607,9 @@ oop G1ParScanThreadState::handle_evacuation_failure_par(oop old, markWord m, siz
if (forward_ptr == NULL) {
// Forward-to-self succeeded. We are the "owner" of the object.
HeapRegion* r = _g1h->heap_region_containing(old);
// Records evac failure objs, this will help speed up iteration
// of these objs later in *remove self forward* phase of post evacuation.
r->record_evac_failure_obj(old);

if (_evac_failure_regions->record(r->hrm_index())) {
_g1h->hr_printer()->evac_failure(r);