-
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
Changes from all commits
a62d67c
5bc53cf
c0c4135
0b76b05
db9724a
38b2511
3086fd3
c53c4f2
ddd3d6d
ba71462
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 |
|---|---|---|
|
|
@@ -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?
Contributor
Author
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. I can change the sequencing, though I'm not sure it's necessary. Old marking shouldn't be following references into the young generation, so updating references in old that point to young shouldn't matter to old marking.
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. Okay. |
||
| // 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); | ||
| } | ||
| } | ||
| } | ||
|
|
||
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!