RFR: 8315044: GenShen: Verifier detects clean card should be dirty [v4]

[Y. Srinivas Ramakrishna]

On Wed, 30 Aug 2023 18:51:46 GMT, Kelvin Nilsen <kdnilsen at openjdk.org> wrote:

>> When a Reference object is newly discovered, it is placed onto the worker's thread-local discovered list.  This sometimes results in a reference from an old object to a young object, requiring that the remembered set card-table entry be marked as dirty.  This patch causes the marking to be performed.
>
> Kelvin Nilsen has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Replace is_generational with ShenandoahCardBarrier test
>  - Merge remote-tracking branch 'origin/master' into ref-processor-updates-remembered-set
>  - Merge remote-tracking branch 'origin/master' into ref-processor-updates-remembered-set
>  - Also update card table when moving discovered References to global list
>  - Fix whitespace
>  - Mark card as dirty if discovered reference list has interesting pointer

changes look good; minor suggestion on some potential refactor/consolidation with existing card marking code.

src/hotspot/share/gc/shenandoah/shenandoahReferenceProcessor.cpp line 387:

> 385:       T* addr = reinterpret_cast<T*>(java_lang_ref_Reference::discovered_addr_raw(reference));
> 386:       if (heap->is_in_old(addr) && heap->is_in_young(discovered_head)) {
> 387:         heap->mark_card_as_dirty(addr);

Can you instead call the existing `card_mark_barrier` method?
At that time, could also look at the existing uses of that method and refactor them to use the `ShenandoahCardBarrier` test like you did above, and elide the generational check in the body of the method itself, replacing it with an assertion (if you want, that the heap is generational).

(PS: I was also thinking that in the fullness of time it might be worth doing an audit of the uses of `cas` and `set_oop` and making sure they would call the card-marking code instead of exposing the functionality at the call-sites like done here. Callers that want to avoid card-marking would need to call the respective `_raw` versions then. We should makr sure to conform to naming conventions for those methods that are consistent with those used in other parts of jvm/gc code. This can be done separately and might affect legacy Shenandoah code as well. Just leaving this comment here so as not to lose the thought, but I doubt we want to do that locally here in this PR.)

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

PR Review: https://git.openjdk.org/shenandoah/pull/314#pullrequestreview-1605940006
PR Review Comment: https://git.openjdk.org/shenandoah/pull/314#discussion_r1312414190