On Thu, 19 Feb 2026 16:08:40 GMT, Ivan Walulya <[email protected]> wrote:
> Hi, > > Please review this change to add Array Chunking for flatArrays with oops > across the G1, Parallel, and Serial GCs > > This update addresses the regression reported in > [JDK-8373029](https://bugs.openjdk.org/browse/JDK-8373029). > > Results after the change: > > > jdk/bin/java BigArray 80000000 > > real 0m35.790s > user 5m38.042s > sys 0m1.666s > > > > jdk/bin/java --enable-preview BigArray 80000000 > > real 0m18.865s > user 2m57.155s > sys 0m1.799s > > > Test: Tier 1-2 with` --enable-preview` I think this looks good. I have a couple of questions and cleanups. ## Thoughts for future RFEs I think there is room for improvement with our `_specialized` functions. I think the `const address oop_addr` pointing at where the header would be if it had a header is a bit of a gotcha you need to realise when reading the code. (Not sure `InlineKlass::oop_iterate_specialized` is used at all after this change) I think we might be able to use the ValuePayload abstraction here (once it is checked in) to make that code clearer. Also now we have this uintptr_t -> void* -> uintptr_t for the bounded version. Might just want to just pick `uintptr_t` as the external type and not go back and forth internally. src/hotspot/share/gc/serial/serialFullGC.cpp line 397: > 395: FlatArrayKlass* faklass = FlatArrayKlass::cast(array->klass()); > 396: mark_and_push_closure.do_klass(faklass->element_klass()); > 397: } We have talked about this at the office a couple of times, but no sure if we reached any understanding. But why do we feel the need for following the element klass in the flat array case. I understand conceptually that there are no headers with the element klass to visit here, but how is this different from an empty or all null reference array? _PS: I see this code in Serial and G1, but not sure where to find it in Parallel if it exists._ _PPS: Also what about nested flattened values, (not sure if they occur with oops in the wild, yet) but it does not seem like we should rely on reaching the metadata this way. We should ensure that visiting the containers metadata will visit all its flattened fields/elements metadata._ src/hotspot/share/oops/flatArrayKlass.inline.hpp line 51: > 49: precond(contains_oops()); > 50: precond(start >= 0); > 51: assert(start <= end, "Invalid range [%d - %d)", start, end); Suggestion: assert(start <= end, "Invalid range [%d - %d)", start, end); src/hotspot/share/oops/flatArrayKlass.inline.hpp line 55: > 53: > 54: const int shift = > Klass::layout_helper_log2_element_size(layout_helper()); > 55: const int addr_incr = 1 << shift; Unused. Suggestion: ------------- Marked as reviewed by aboldtch (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2142#pullrequestreview-3830788368 PR Review Comment: https://git.openjdk.org/valhalla/pull/2142#discussion_r2832116320 PR Review Comment: https://git.openjdk.org/valhalla/pull/2142#discussion_r2832086528 PR Review Comment: https://git.openjdk.org/valhalla/pull/2142#discussion_r2832087523
