On Wed, 4 Mar 2026 16:56:22 GMT, Axel Boldt-Christmas <[email protected]> wrote:
>> `obj_at(int)` on `flatArrayOopDesc` should never be called as it may fail >> with an out of memory error and crash. Similarly `objArrayOopDesc` should >> not provide a shared interface to this member function which is only valid >> to call on a `refArrayOopDesc`. >> >> This RFE cleans up the last potentially invalid uses by using and asserting >> stricter types at the call sites. >> >> Most cleanup are straight forward, however there are a few parts where some >> extra feedback is appreciated. >> >> First the primate caches (ac32b67e811945ed474ac62d07275e8a464e72b9). >> [JDK-8369921](https://bugs.openjdk.org/browse/JDK-8369921)/#1685 conditioned >> the use of the caches behind the `!PreviewFeatures.isEnabled()` such that >> they are not used when running with Valhalla. However it did not do this for >> Character, even though both the issue and the PR mentions this Class as >> well, and could not find any discussion in the PR. >> >> Update: @jsikstro pointed me to >> [JDK-8372619](https://bugs.openjdk.org/browse/JDK-8372619), I am curious to >> where and how we access these caches (except for `valueOf`), as this change >> which removes the caches in Valhalla seems to run through our testing. >> >> Second the caches were still created even if they were not used. In this PR >> I chose to not create these caches at all if we are in preview mode. Also I >> am not sure if it is the case that we always trust the internal final values >> and do fold them, but I changed it to use `@stable` checks vs a sentinel, >> which I know are trusted as final. However maybe this is unnecessary, and >> some core library / compiler person has some input. >> >> Second is all the JVMCI changes (fe98ae0dc03c99c770dad1364fb1f010ea1ee7bf). >> This patch changes it so that the internal JVMCI Classes that that hotspot >> knows about are required to be RefArrays (this was already the assumption, >> but this hardens this by changing their types). And for external arrays they >> either throw an exception if it is an unexpected type (the case for >> `executeHotSpotNmethod`) or use `obj_at(int, TRAP)` and returns with an out >> of memory error. >> >> Testing: >> * Running tier 1-7 with `--enable-preview' > > Axel Boldt-Christmas has updated the pull request with a new target base due > to a merge or a rebase. The pull request now contains 16 commits: > > - Deleted obj_at and added obj_at_is_null > - Merge remote-tracking branch 'upstream_valhalla/lworld' into > lworld-remove-obj-at > - Merge remote-tracking branch 'upstream_valhalla/lworld' into > lworld-remove-obj-at > - Remove unused obj_at > - HotSpotJVMCI should use RefArrayOops internally, and expect exceptions > externally > - Fix ZERO > - Fix jvmti_GetThreadGroupChildren > - Fix ForeignGlobals > - Fix java_lang_invoke_MethodType > - Fix oop_verify_on > - ... and 6 more: > https://git.openjdk.org/valhalla/compare/a6b20a25...e417b2a5 I looked through the C++ code (except JVMCI) and I think this looks good. I have one question below: src/hotspot/share/runtime/deoptimization.cpp line 1148: > 1146: static BoxCache<PrimitiveType, CacheType, BoxType> *_singleton; > 1147: BoxCache(Thread* thread) { > 1148: assert(!Arguments::enable_preview(), "Should not use box caches > with enable preview"); Should this be the more descriptive check or is there a reason why enable_preview() is more correct? (This comment goes to other additions as well): Suggestion: assert(!Arguments::is_valhalla_enabled(), "Should not use box caches with enable preview"); ------------- PR Review: https://git.openjdk.org/valhalla/pull/2157#pullrequestreview-3896711685 PR Review Comment: https://git.openjdk.org/valhalla/pull/2157#discussion_r2890129359
