RFR: 8300915: G1: incomplete SATB because nmethod entry barriers don't get armed [v4]

[Thomas Schatzl]

On Thu, 26 Jan 2023 11:20:35 GMT, Richard Reingruber <rrich at openjdk.org> wrote:

> > Hmm. Some thoughts. The way I look at it, we have essentially 4 different
> > endings to CM when we poke it to "start". 1) successful finish, 2) undone end
> > (no CM needed in the end), 3) aborted by STW full GC, 4) aborted by new CM
> > request. If I understand this patch correctly, it tries to recognize case 2 as
> > essentiallly being like 1, in the sense that we have traversed the full heap
> > and we are done. Is that correct?
> 
> Well it can be seen like this.
> 
> I wasn't aware of 4). Does it really exist? The assertion in `CodeCache::on_gc_marking_cycle_start()` should have fired here:

Case 4 isn't possible. During an existing marking (from start to the Remark pause), the `G1CollectorState::initiate_conc_mark_if_possible` flag prevents that. The related code to set that is fairly strewn around `G1Policy` though, I agree.

> 
> ```
> CodeCache::on_gc_marking_cycle_start() : void
> G1CollectedHeap::start_codecache_marking_cycle() : void
> G1ConcurrentMark::pre_concurrent_start(enum GCCause::Cause) : void
> G1YoungCollector::pre_evacuate_collection_set(G1EvacInfo *) : void
> G1YoungCollector::collect() : void
> ```
> 
> I havn't seen it so far though.

Neither have we.

> 
> Maybe you mean the restart of marking in `G1ConcurrentMarkThread::phase_mark_loop()`? This is not problematic I think.
> 
> > In that case, I think there is still the same issue for case 4. If a CM is
> > aborted because we are interrupting an old CM in order to start a new CM, then
> > we still have a case where we have an old marking cycle that has finished, but
> > not successfully. We can't call
> > G1CollectedHeap::finish_codecache_marking_cycle() until we have successfully
> > marked through all objects in the heap. If we were to call that when an old CM
> > is aborted, then is_unloading() might start returning true on nmethods that
> > were on-stack in some loom continuation object in the old generation. This was
> > the main point of only conditionally starting cycles - then the epoch counters
> > being marked in the nmethods would be conservative and potentially keep
> > nmethods around a bit longer that really could be discarded.
> 
> That's a very good point I didn't see and didn't get it from the comment I removed.

Maybe we can improve the comment instead of removing then? :)

Thomas

-------------

PR: https://git.openjdk.org/jdk/pull/12194