RFR: 8368811: [Leyden] Use AOTRuntimeConstants table for card_table::_byte_map_base
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...) To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it. I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code. @adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there. Changes were testing tier1-5. ------------- Commit messages: - 8368811: [Leyden] Use AOTRuntimeConstants table for card_table::_byte_map_base Changes: https://git.openjdk.org/leyden/pull/102/files Webrev: https://webrevs.openjdk.org/?repo=leyden&pr=102&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8368811 Stats: 355 lines in 18 files changed: 203 ins; 113 del; 39 mod Patch: https://git.openjdk.org/leyden/pull/102.diff Fetch: git fetch https://git.openjdk.org/leyden.git pull/102/head:pull/102 PR: https://git.openjdk.org/leyden/pull/102
On Sat, 27 Sep 2025 23:55:51 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
@adinn what do you think about card_shift in AOTRuntimeConstants table? You added it there.
I added it because 1. When I added the AOTRuntimeConstants area to cater for ergonomic changes to RegionGrainSize I thought it might be enlightening to add a 2nd constant as a proof of the concept 2. The card shift is controlled by a GC command line option that users might in theory (but probably not in practice) wish to change between training/assembly and production runs. I'm not committed to keeping it in AOTRuntimeConstants. I agree we can simply log the Assembly time flag and then enforce the same setting in production. ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3352742051
On Tue, 30 Sep 2025 15:26:32 GMT, Andrew Dinn <adinn@openjdk.org> wrote:
I'm not committed to keeping it in AOTRuntimeConstants. I agree we can simply log the Assembly time flag and then enforce the same setting in production.
Thank you. My main concern is that it adds a lot of code to barriers. I will replace it with flag check. Thanks! ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3352905619
On Sat, 27 Sep 2025 23:55:51 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
And I need to fix minimal VM build :(. ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3352906735
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Removed card_shift from AOT constants table. Fixed minimal VM build. ------------- Changes: - all: https://git.openjdk.org/leyden/pull/102/files - new: https://git.openjdk.org/leyden/pull/102/files/a1721c98..e94fb605 Webrevs: - full: https://webrevs.openjdk.org/?repo=leyden&pr=102&range=01 - incr: https://webrevs.openjdk.org/?repo=leyden&pr=102&range=00-01 Stats: 244 lines in 16 files changed: 37 ins; 175 del; 32 mod Patch: https://git.openjdk.org/leyden/pull/102.diff Fetch: git fetch https://git.openjdk.org/leyden.git pull/102/head:pull/102 PR: https://git.openjdk.org/leyden/pull/102
On Thu, 2 Oct 2025 05:30:02 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
Removed card_shift from AOT constants table. Fixed minimal VM build.
Looks ok, modulo a couple of optional suggested tweaks and a name correction for the arm failure. I'm not sure why we are seeing a Shenandoah timeout but it ought to be unrelated as Shenandoah barriers are 1) independent of the (G1) region grain size and 2) access the card table base via a thread local. src/hotspot/cpu/aarch64/gc/shared/cardTableBarrierSetAssembler_aarch64.cpp line 48:
46: if (UseCondCardMark) { 47: Label L_already_dirty; 48: __ ldrb(rscratch2, Address(obj, rscratch));
Do we need an extra precondition? `precond(!UseCondCardMark || rscratch != rscratch2)` src/hotspot/share/gc/shared/c1/cardTableBarrierSetC1.cpp line 68:
66: 67: #ifdef CARDTABLEBARRIERSET_POST_BARRIER_HELPER 68: assert(!aotCodeCache::is_on(), "this path is not implemented");
Suggestion: assert(!AOTCodeCache::is_on(), "this path is not implemented"); src/hotspot/share/gc/shared/c1/cardTableBarrierSetC1.cpp line 88:
86: __ move(byte_map_base_adr, byte_map_base_reg); 87: LIR_Address* byte_map_base_indirect = new LIR_Address(byte_map_base_reg, 0, T_LONG); 88: //LIR_Opr byte_map_base = gen->new_pointer_register();
Do we need this comment? ------------- Marked as reviewed by adinn (Committer). PR Review: https://git.openjdk.org/leyden/pull/102#pullrequestreview-3293207725 PR Review Comment: https://git.openjdk.org/leyden/pull/102#discussion_r2397670560 PR Review Comment: https://git.openjdk.org/leyden/pull/102#discussion_r2397781476 PR Review Comment: https://git.openjdk.org/leyden/pull/102#discussion_r2397735318
On Thu, 2 Oct 2025 08:43:25 GMT, Andrew Dinn <adinn@openjdk.org> wrote:
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
Removed card_shift from AOT constants table. Fixed minimal VM build.
src/hotspot/share/gc/shared/c1/cardTableBarrierSetC1.cpp line 68:
66: 67: #ifdef CARDTABLEBARRIERSET_POST_BARRIER_HELPER 68: assert(!aotCodeCache::is_on(), "this path is not implemented");
Suggestion:
assert(!AOTCodeCache::is_on(), "this path is not implemented");
Goot catch. 32-bit ARM build failed because of this.
src/hotspot/share/gc/shared/c1/cardTableBarrierSetC1.cpp line 88:
86: __ move(byte_map_base_adr, byte_map_base_reg); 87: LIR_Address* byte_map_base_indirect = new LIR_Address(byte_map_base_reg, 0, T_LONG); 88: //LIR_Opr byte_map_base = gen->new_pointer_register();
Do we need this comment?
Removed. I experimented with separate registers but reusing register (byte_map_base_reg) seems work too. ------------- PR Review Comment: https://git.openjdk.org/leyden/pull/102#discussion_r2399240598 PR Review Comment: https://git.openjdk.org/leyden/pull/102#discussion_r2399246826
On Thu, 2 Oct 2025 08:50:53 GMT, Andrew Dinn <adinn@openjdk.org> wrote:
I'm not sure why we are seeing a Shenandoah timeout but it ought to be unrelated as Shenandoah barriers are 1) independent of the (G1) region grain size and 2) access the card table base via a thread local.
I did not look on Shenandoah barriers - there could be issues there. There test throws OOM: stderr: [Exception in thread "Thread-0" java.lang.OutOfMemoryError: Java heap space 2025-10-02T06:56:37.6700940Z at TestThreadFailure$NastyThread.run(TestThreadFailure.java:49) 2025-10-02T06:56:37.6701860Z Exception in thread "Thread-1" java.lang.OutOfMemoryError: Java heap space 2025-10-02T06:56:37.6702380Z at TestThreadFailure$NastyThread.run(TestThreadFailure.java:49) ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3361957950
On Thu, 2 Oct 2025 08:18:31 GMT, Andrew Dinn <adinn@openjdk.org> wrote:
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
Removed card_shift from AOT constants table. Fixed minimal VM build.
src/hotspot/cpu/aarch64/gc/shared/cardTableBarrierSetAssembler_aarch64.cpp line 48:
46: if (UseCondCardMark) { 47: Label L_already_dirty; 48: __ ldrb(rscratch2, Address(obj, rscratch));
Do we need an extra precondition? `precond(!UseCondCardMark || rscratch != rscratch2)`
added ------------- PR Review Comment: https://git.openjdk.org/leyden/pull/102#discussion_r2399316948
On Thu, 2 Oct 2025 05:30:02 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
Removed card_shift from AOT constants table. Fixed minimal VM build.
I looked on Shenandoah barriers cpu specific code and don't see any issues. As you said, it loads card table base via a thread local and use constant for card_shift (so added flag check will work for it too). ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3361995474
On Sat, 27 Sep 2025 23:55:51 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
I updated changes. ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3359165920
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision: Addressed comments ------------- Changes: - all: https://git.openjdk.org/leyden/pull/102/files - new: https://git.openjdk.org/leyden/pull/102/files/e94fb605..469f5f7d Webrevs: - full: https://webrevs.openjdk.org/?repo=leyden&pr=102&range=02 - incr: https://webrevs.openjdk.org/?repo=leyden&pr=102&range=01-02 Stats: 3 lines in 2 files changed: 1 ins; 1 del; 1 mod Patch: https://git.openjdk.org/leyden/pull/102.diff Fetch: git fetch https://git.openjdk.org/leyden.git pull/102/head:pull/102 PR: https://git.openjdk.org/leyden/pull/102
On Thu, 2 Oct 2025 16:07:23 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
Addressed comments
Still looks good. ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3363244920
On Thu, 2 Oct 2025 21:38:18 GMT, Andrew Dinn <adinn@openjdk.org> wrote:
Vladimir Kozlov has updated the pull request incrementally with one additional commit since the last revision:
Addressed comments
Still looks good.
Thank you @adinn I will wait for Ioi's mainline merge to premain to integrate this. ------------- PR Comment: https://git.openjdk.org/leyden/pull/102#issuecomment-3363314081
On Sat, 27 Sep 2025 23:55:51 GMT, Vladimir Kozlov <kvn@openjdk.org> wrote:
Currently we use `Relocation` info to patch `card_table::_byte_map_base` referenced in compiled code. This is not completely correct since this is not address. We currently have special check in `AOTCodeCache::init2()` to skip AOT code generation and usage if `byte_map_base` is not relocatable [aotCodeCache.cpp#L341](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/code/aotCod...)
To avoid this we should use existing `AOTRuntimeConstants` table to load `byte_map_base` from it.
I also added few missing loads `card_shift` from `AOTRuntimeConstants` table. Actually I am not sure why we have it in `AOTRuntimeConstants` table. From what I see, it is based on `GCCardSizeInBytes` flag [cardTable.cpp#L46](https://github.com/openjdk/leyden/blob/premain/src/hotspot/share/gc/shared/c...) and never modified. The flags is not adjusted ergonomically. So we can simple record the flag in AOT code configuration and verify it when loading AOT code.
@adinn what do you think about `card_shift` in `AOTRuntimeConstants` table? You added it there.
Changes were testing tier1-5.
This pull request has now been integrated. Changeset: f155270f Author: Vladimir Kozlov <kvn@openjdk.org> URL: https://git.openjdk.org/leyden/commit/f155270fc4a1852c50c732f45d8c80f417a11b... Stats: 232 lines in 17 files changed: 72 ins; 120 del; 40 mod 8368811: [Leyden] Use AOTRuntimeConstants table for card_table::_byte_map_base Reviewed-by: adinn ------------- PR: https://git.openjdk.org/leyden/pull/102
participants (2)
-
Andrew Dinn
-
Vladimir Kozlov