-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8370039: GenShen: array copy SATB barrier improvements #28183
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
Open
earthling-amzn
wants to merge
10
commits into
openjdk:master
Choose a base branch
from
earthling-amzn:satb-fixes
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+18
−59
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
a62d67c
Do not unconditionally enqueue old objects during marking
earthling-amzn 5bc53cf
Merge remote-tracking branch 'jdk/master' into satb-fixes
earthling-amzn c0c4135
Simplify arraycopy barrier case for marking
earthling-amzn 0b76b05
Why does this break?
earthling-amzn db9724a
Merge remote-tracking branch 'jdk/master' into satb-fixes
earthling-amzn 38b2511
Merge remote-tracking branch 'jdk/master' into satb-fixes
earthling-amzn 3086fd3
More simplification of arraycopy mark barrier
earthling-amzn c53c4f2
We can also filter out old when striclty marking young
earthling-amzn ddd3d6d
SATB barrier for old must be independent of young collection gc state
earthling-amzn ba71462
Revert "We can also filter out old when striclty marking young"
earthling-amzn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -387,13 +387,11 @@ template <class T, bool HAS_FWD, bool EVAC, bool ENQUEUE> | |
| void ShenandoahBarrierSet::arraycopy_work(T* src, size_t count) { | ||
| // Young cycles are allowed to run when old marking is in progress. When old marking is in progress, | ||
| // this barrier will be called with ENQUEUE=true and HAS_FWD=false, even though the young generation | ||
| // may have forwarded objects. In this case, the `arraycopy_work` is first called with HAS_FWD=true and | ||
| // ENQUEUE=false. | ||
| assert(HAS_FWD == _heap->has_forwarded_objects() || _heap->is_concurrent_old_mark_in_progress(), | ||
| "Forwarded object status is sane"); | ||
| // may have forwarded objects. | ||
| assert(HAS_FWD == _heap->has_forwarded_objects() || _heap->is_concurrent_old_mark_in_progress(), "Forwarded object status is sane"); | ||
| // This function cannot be called to handle marking and evacuation at the same time (they operate on | ||
| // different sides of the copy). | ||
| assert((HAS_FWD || EVAC) != ENQUEUE, "Cannot evacuate and mark both sides of copy."); | ||
| static_assert((HAS_FWD || EVAC) != ENQUEUE, "Cannot evacuate and mark both sides of copy."); | ||
|
|
||
| Thread* thread = Thread::current(); | ||
| SATBMarkQueue& queue = ShenandoahThreadLocalData::satb_mark_queue(thread); | ||
|
|
@@ -412,7 +410,7 @@ void ShenandoahBarrierSet::arraycopy_work(T* src, size_t count) { | |
| shenandoah_assert_forwarded_except(elem_ptr, obj, _heap->cancelled_gc()); | ||
| ShenandoahHeap::atomic_update_oop(fwd, elem_ptr, o); | ||
| } | ||
| if (ENQUEUE && !ctx->is_marked_strong_or_old(obj)) { | ||
| if (ENQUEUE && !ctx->is_marked_strong(obj)) { | ||
| _satb_mark_queue_set.enqueue_known_active(queue, obj); | ||
| } | ||
| } | ||
|
|
@@ -426,68 +424,29 @@ void ShenandoahBarrierSet::arraycopy_barrier(T* src, T* dst, size_t count) { | |
| return; | ||
| } | ||
|
|
||
| char gc_state = ShenandoahThreadLocalData::gc_state(Thread::current()); | ||
| const char gc_state = ShenandoahThreadLocalData::gc_state(Thread::current()); | ||
| if ((gc_state & ShenandoahHeap::MARKING) != 0) { | ||
|
Member
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. A question on sequencing. Can we have marking and update-refs at the same time? If so, should update-refs happen first, and then marking should be doing the walk over (fixed-up) objects? |
||
| // If marking old or young, we must evaluate the SATB barrier. This will be the only | ||
| // action if we are not marking old. If we are marking old, we must still evaluate the | ||
| // load reference barrier for a young collection. | ||
| arraycopy_marking(dst, count); | ||
| } | ||
|
|
||
| if ((gc_state & ShenandoahHeap::EVACUATION) != 0) { | ||
| assert((gc_state & ShenandoahHeap::YOUNG_MARKING) == 0, "Cannot be marking young during evacuation"); | ||
| arraycopy_evacuation(src, count); | ||
| } else if ((gc_state & ShenandoahHeap::UPDATE_REFS) != 0) { | ||
| assert((gc_state & ShenandoahHeap::YOUNG_MARKING) == 0, "Cannot be marking young during update-refs"); | ||
| arraycopy_update(src, count); | ||
| } | ||
|
|
||
| if (_heap->mode()->is_generational()) { | ||
| assert(ShenandoahSATBBarrier, "Generational mode assumes SATB mode"); | ||
| if ((gc_state & ShenandoahHeap::YOUNG_MARKING) != 0) { | ||
| arraycopy_marking(src, dst, count, false); | ||
| } | ||
| if ((gc_state & ShenandoahHeap::OLD_MARKING) != 0) { | ||
| arraycopy_marking(src, dst, count, true); | ||
| } | ||
| } else if ((gc_state & ShenandoahHeap::MARKING) != 0) { | ||
| arraycopy_marking(src, dst, count, false); | ||
| } | ||
| } | ||
|
|
||
| template <class T> | ||
| void ShenandoahBarrierSet::arraycopy_marking(T* src, T* dst, size_t count, bool is_old_marking) { | ||
| void ShenandoahBarrierSet::arraycopy_marking(T* dst, size_t count) { | ||
| assert(_heap->is_concurrent_mark_in_progress(), "only during marking"); | ||
| /* | ||
| * Note that an old-gen object is considered live if it is live at the start of OLD marking or if it is promoted | ||
| * following the start of OLD marking. | ||
| * | ||
| * 1. Every object promoted following the start of OLD marking will be above TAMS within its old-gen region | ||
| * 2. Every object live at the start of OLD marking will be referenced from a "root" or it will be referenced from | ||
| * another live OLD-gen object. With regards to old-gen, roots include stack locations and all of live young-gen. | ||
| * All root references to old-gen are identified during a bootstrap young collection. All references from other | ||
| * old-gen objects will be marked during the traversal of all old objects, or will be marked by the SATB barrier. | ||
| * | ||
| * During old-gen marking (which is interleaved with young-gen collections), call arraycopy_work() if: | ||
| * | ||
| * 1. The overwritten array resides in old-gen and it is below TAMS within its old-gen region | ||
| * 2. Do not call arraycopy_work for any array residing in young-gen because young-gen collection is idle at this time | ||
| * | ||
| * During young-gen marking, call arraycopy_work() if: | ||
| * | ||
| * 1. The overwritten array resides in young-gen and is below TAMS within its young-gen region | ||
| * 2. Additionally, if array resides in old-gen, regardless of its relationship to TAMS because this old-gen array | ||
| * may hold references to young-gen | ||
| */ | ||
| if (ShenandoahSATBBarrier) { | ||
| T* array = dst; | ||
| HeapWord* array_addr = reinterpret_cast<HeapWord*>(array); | ||
| ShenandoahHeapRegion* r = _heap->heap_region_containing(array_addr); | ||
| if (is_old_marking) { | ||
| // Generational, old marking | ||
| assert(_heap->mode()->is_generational(), "Invariant"); | ||
| if (r->is_old() && (array_addr < _heap->marking_context()->top_at_mark_start(r))) { | ||
| arraycopy_work<T, false, false, true>(array, count); | ||
| } | ||
| } else if (_heap->mode()->is_generational()) { | ||
| // Generational, young marking | ||
| if (r->is_old() || (array_addr < _heap->marking_context()->top_at_mark_start(r))) { | ||
| arraycopy_work<T, false, false, true>(array, count); | ||
| } | ||
| } else if (array_addr < _heap->marking_context()->top_at_mark_start(r)) { | ||
| // Non-generational, marking | ||
| arraycopy_work<T, false, false, true>(array, count); | ||
| if (!_heap->marking_context()->allocated_after_mark_start(reinterpret_cast<HeapWord*>(dst))) { | ||
| arraycopy_work<T, false, false, true>(dst, count); | ||
| } | ||
| } | ||
| } | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is the comment at the start of arraycopy_work() still relevant? The description of the PR suggests that we will no longer call arraycopy_work() twice, but I'm not sure I fully understand the context of that statement.
Uh oh!
There was an error while loading. Please reload this page.
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.
We used to call this twice (as
arraycopy_markingfor each object if young and old marking were both in progress). The comment at the start is trying to explain how thearraycopy_workcould be called with different template parameters depending on thegc_state. It is confusingly worded, but I believe the comment is correct and the code is wrong here. The way the code is in the PR, it would effectively turn off the old SATB during young evacuation and young update-refs, which is not what we want. Good catch!