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.

Sign up for GitHub

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

Jump to bottom

8273476: G1: refine G1CollectedHeap::par_iterate_regions_array_part_from #5414

Closed
wants to merge 5 commits into from
Closed

8273476: G1: refine G1CollectedHeap::par_iterate_regions_array_part_from #5414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6292fd2
refine par_iterate_regions_array_part_from
Hamlin-Li Sep 8, 2021
78da4ce
refine comments and code
Hamlin-Li Sep 9, 2021
489947a
Fix assert
Hamlin-Li Sep 9, 2021
46d5a3c
Fix issue
Hamlin-Li Sep 9, 2021
af020d6
Refine comments
Hamlin-Li Sep 9, 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
13 changes: 6 additions & 7 deletions src/hotspot/share/gc/g1/g1CollectedHeap.cpp
View file
Open in desktop
Expand Up @@ -2326,12 +2326,11 @@ void G1CollectedHeap::collection_set_iterate_increment_from(HeapRegionClosure *c
_collection_set.iterate_incremental_part_from(cl, hr_claimer, worker_id);
}

void G1CollectedHeap::par_iterate_regions_array_part_from(HeapRegionClosure* cl,
HeapRegionClaimer* hr_claimer,
const uint* regions,
size_t offset,
size_t length,
uint worker_id) const {
void G1CollectedHeap::par_iterate_regions_array(HeapRegionClosure* cl,
HeapRegionClaimer* hr_claimer,
const uint regions[],
size_t length,
uint worker_id) const {
assert_at_safepoint();
if (length == 0) {
return;
Expand All @@ -2342,7 +2341,7 @@ void G1CollectedHeap::par_iterate_regions_array_part_from(HeapRegionClosure* cl,
size_t cur_pos = start_pos;

do {
uint region_idx = regions[cur_pos + offset];
uint region_idx = regions[cur_pos];
if (hr_claimer == NULL || hr_claimer->claim_region(region_idx)) {
HeapRegion* r = region_at(region_idx);
bool result = cl->do_heap_region(r);
Expand Down
15 changes: 7 additions & 8 deletions src/hotspot/share/gc/g1/g1CollectedHeap.hpp
View file
Open in desktop
Expand Up @@ -1155,15 +1155,14 @@ class G1CollectedHeap : public CollectedHeap {
collection_set_iterate_increment_from(blk, NULL, worker_id);
}
void collection_set_iterate_increment_from(HeapRegionClosure *blk, HeapRegionClaimer* hr_claimer, uint worker_id);
// Iterate part of an array of region indexes given by offset and length, applying
// Iterate over the array of region indexes, uint regions[length], applying
// the given HeapRegionClosure on each region. The worker_id will determine where
// in the part to start the iteration to allow for more efficient parallel iteration.
void par_iterate_regions_array_part_from(HeapRegionClosure* cl,
HeapRegionClaimer* hr_claimer,
const uint* regions,
size_t offset,
size_t length,
uint worker_id) const;
// to start the iteration to allow for more efficient parallel iteration.
void par_iterate_regions_array(HeapRegionClosure* cl,
HeapRegionClaimer* hr_claimer,
const uint regions[],
size_t length,
uint worker_id) const;

// Returns the HeapRegion that contains addr. addr must not be NULL.
template <class T>
Expand Down
6 changes: 5 additions & 1 deletion src/hotspot/share/gc/g1/g1CollectionSet.cpp
View file
Open in desktop
Expand Up @@ -232,7 +232,11 @@ void G1CollectionSet::iterate_part_from(HeapRegionClosure* cl,
size_t offset,
size_t length,
uint worker_id) const {
_g1h->par_iterate_regions_array_part_from(cl, hr_claimer, _collection_set_regions, offset, length, worker_id);
_g1h->par_iterate_regions_array(cl,
hr_claimer,
&_collection_set_regions[offset],
length,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be length - offset, shouldn't it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
length,
length - offset,

Untested :)

Copy link
Author

@Hamlin-Li Hamlin-Li Sep 9, 2021 •
edited

Choose a reason for hiding this comment

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

I thought so too at first, but seems it's not.
The passed in length in both G1CollectionSet::par_iterate/iterate_incremental_part_from are both the length of the "sub" array already. ( increment_length() is passed as length in iterate_incremental_part_from)
How do you think about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right.

worker_id);
}

void G1CollectionSet::update_young_region_prediction(HeapRegion* hr,
Expand Down
11 changes: 5 additions & 6 deletions src/hotspot/share/gc/g1/g1EvacFailureRegions.cpp
View file
Open in desktop
Expand Up @@ -49,12 +49,11 @@ void G1EvacFailureRegions::initialize(uint max_regions) {
void G1EvacFailureRegions::par_iterate(HeapRegionClosure* closure,
HeapRegionClaimer* _hrclaimer,
uint worker_id) {
G1CollectedHeap::heap()->par_iterate_regions_array_part_from(closure,
_hrclaimer,
_evac_failure_regions,
0,
Atomic::load(&_evac_failure_regions_cur_length),
worker_id);
G1CollectedHeap::heap()->par_iterate_regions_array(closure,
_hrclaimer,
_evac_failure_regions,
Atomic::load(&_evac_failure_regions_cur_length),
worker_id);
}

void G1EvacFailureRegions::reset() {
Expand Down