RFR: 8323809: Serial: Refactor card table verification [v2]

[Thomas Schatzl]

On Fri, 19 Jan 2024 17:43:50 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> Simple refactoring and remove much outdated comments.
>
> Albert Mingkun Yang 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 one additional commit since the last revision:
> 
>   s1-card-verify

I did not really try to understand the original code in complete detail, but focused myselves on the new code where the point of the verification is that for every old-to-young reference there must be a dirty card mark.
In particular it is unclear to me how the code handles imprecise marks.

src/hotspot/share/gc/serial/cardTableRS.cpp line 82:

> 80:   template <class T> void do_oop_work(T* p) {
> 81:     oop obj = RawAccess<>::oop_load(p);
> 82:     if (_young_gen->is_in_reserved(obj) &&

I would prefer, if this code also failed on values outside of the heap apart from `nullptr` for `obj`, but I understand it's not he point of this particular verification code.

src/hotspot/share/gc/serial/cardTableRS.cpp line 83:

> 81:     oop obj = RawAccess<>::oop_load(p);
> 82:     if (_young_gen->is_in_reserved(obj) &&
> 83:         !_card_table->is_dirty_for_addr(p)) {

Is this correct for imprecise marks at e.g. before-gc verification?

In this case `p` might be on a different card than the mark. The original code seems to handle this case (although clumsily afaict). Or am I missing something?

How did you test this change?

src/hotspot/share/gc/serial/cardTableRS.cpp line 98:

> 96: 
> 97:   void do_oop(oop* p)       override { CheckForUnmarkedOops::do_oop_work(p); }
> 98:   void do_oop(narrowOop* p) override { CheckForUnmarkedOops::do_oop_work(p); }

Suggestion:

  void do_oop(oop* p)       override { do_oop_work(p); }
  void do_oop(narrowOop* p) override { do_oop_work(p); }


Should work...

src/hotspot/share/gc/serial/cardTableRS.cpp line 122:

> 120:       if (object_check.has_unmarked_oop()) {
> 121:         guarantee(_card_table->is_dirty_for_addr(obj), "Found unmarked old-to-young pointer");
> 122:       }

Suggestion:

      guarantee(!object_check.has_unmarked_oop(), "Found unmarked old-to-young pointer");


This looks much more concise, there is no need to repeat the check imo. Please also add some information about the failing address into the message.

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

Changes requested by tschatzl (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17447#pullrequestreview-1836717551
PR Review Comment: https://git.openjdk.org/jdk/pull/17447#discussion_r1462012677
PR Review Comment: https://git.openjdk.org/jdk/pull/17447#discussion_r1462031752
PR Review Comment: https://git.openjdk.org/jdk/pull/17447#discussion_r1462036124
PR Review Comment: https://git.openjdk.org/jdk/pull/17447#discussion_r1462015458