RFR: 8330145: Serial: Refactor SerialHeap::scan_evacuated_objs

[Thomas Schatzl]

On Thu, 18 Apr 2024 08:41:42 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> src/hotspot/share/gc/serial/serialHeap.cpp line 793:
>> 
>>> 791:       oop_iterate_from(old_cl, old_gen()->space(), saved_top);
>>> 792:       old_gen()->set_saved_mark_word();
>>> 793:     }
>> 
>> I am really not sure this inlining makes the code more readable. The old code had a nice structure that showed exactly what it has been doing without going too much into detail. Maybe the naming wasn't obvious, but although now obviously this code does the same, the overview is lost imo.
>
> The new code places accesses to saved-mark-word and iteration on the same level, also makes it obvious that one doesn't need to redundant `no_allocs_since_save_marks` call on old-gen.
> 
> What "overview" do you think is lost?

the previous structure is literally:


do {
  process-new-oops-encountered-during-evac-within-young;
  process-new-oops-encountered-during-evac-within-old;
} while (some-new-evacuations-somewhere);

very straightforward, and highlights the essence of this loop at a higher level without exposing too many details about the implementation.
The suggested change adds lots of noise around that for "keeping accesses to mark word at the same level" that imo obfuscates the intent of the code.

Additionally it looks like copy&paste too. Instead of the original code which has one line for every action, the reader needs to first find out the structure of the code by parsing the blocks, and ignoring the save mark handling to find that the code actually iterates over the most recently evacuated objects.

The redundancy in the `no_allocs_...` is imo negligible compared to the loss in at-a-glance clarity about what the method does. After understanding about the general structure what the code does, one would simply drill down deeper (a click away nowadays).

Maybe keeping the structure could be achieved by refactoring the code to a form similar to this:


  bool something-changed;
  do 
    something-changed = false;
    something-changed |= evacuate-oops-since-last(young_gen(), &cl); // or keep the young_gen()->...(&cl); structure.
    something-changed |= evacuate-oops-since-last(old_gen(), &cl);
  } while (something-changed);

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18750#discussion_r1570427345