RFR: 8343468: GenShen: Enable relocation of remembered set card tables [v7]

[Cesar Soares Lucas]

On Wed, 5 Mar 2025 17:32:30 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

>> Cesar Soares Lucas has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains nine commits:
>> 
>>  - Fix merge conflict
>>  - Address PR feedback: formatting.
>>  - Revert changes to shared cardTable.hpp
>>  - Revert changes to shared cardTable.hpp
>>  - Fix merge conflict
>>  - Address PR feedback: no changes to shared files.
>>  - Merge master
>>  - Addressing PR comments: some refactorings, ppc fix, off-by-one fix.
>>  - Relocation of Card Tables
>
> src/hotspot/share/gc/shenandoah/shenandoahCardTable.cpp line 57:
> 
>> 55:   _byte_map_base = _byte_map - (uintptr_t(low_bound) >> _card_shift);
>> 56:   assert(byte_for(low_bound) == &_byte_map[0], "Checking start of map");
>> 57:   assert(byte_for(high_bound-1) <= &_byte_map[last_valid_index()], "Checking end of map");
> 
> It is a bit sad to see these asserts go. Is this because `_byte_map` is now mutable? May I suggest doing something like:
> 
> 
>   _write_byte_map = (CardValue*) write_space.base();
>   _write_byte_map_base = _byte_map - (uintptr_t(low_bound) >> _card_shift);
>   ...later...
>   _read_byte_map = (CardValue*) read_space.base();
>   _read_byte_map_base = _byte_map - (uintptr_t(low_bound) >> _card_shift);
>   ...later...
> 
>   // Set up current byte map
>   _byte_map = _write_byte_map;
>   _byte_map_base = _write_byte_map_base;
> 
>   // Check one side is good
>   assert(byte_for(low_bound) == &_byte_map[0], "Checking start of map");
>   assert(byte_for(high_bound-1) <= &_byte_map[last_valid_index()], "Checking end of map");
>   swap_read_and_write_tables();
> 
>   // Check another side is good
>   assert(byte_for(low_bound) == &_byte_map[0], "Checking start of map");
>   assert(byte_for(high_bound-1) <= &_byte_map[last_valid_index()], "Checking end of map");
>   swap_read_and_write_tables();

Yeah, I didn't like that either. If I recall correctly I had to remove them because part of the expressions ended up calling `byte_map(_base)` which would come from `ThreadLocalData` which wasn't set at the time `initialize()` was being called. Now that we don't have the virtual methods anymore I think I can put back the asserts. I'll try+test that and get back to you.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23170#discussion_r1981983462