On Thu, 23 Apr 2026 19:34:48 GMT, Frederic Parain <[email protected]> wrote:
> Fixes in the logic to determine if a value class is naturally atomic or not. > The previous implementation incorrectly considered values classes with a > single non-empty nullable field as being naturally atomic. > Also includes improvements to the InstanceKlass::is_naturally_atomic() to > prevent bad usages. > > Tested locally. > More testing in progress on Mach5 (tiers 1-4). > > > > --------- > - [X] I confirm that I make this contribution in accordance with the [OpenJDK > Interim AI Policy](https://openjdk.org/legal/ai). Looks good. Me and Axel has been brainstorming about if we could figure out a name that could help us get rid of this new boolean and at the same time clarify the code and help prevent bugs like the one that this PR is fixing. The question whether something is naturally atomic depends on more than the explicit fields of a value, but rather what a specific value payload where the fields are residing contains a null-marker or not. I would like to come up with a name to describe the set of explicit fields described in the value excluding any potential null-marker. I think that could help clarify the code for the readers. We haven't managed to come up with a name like that yet, so let's defer that to the future. I find the style used to implement the new is_naturally_atomic quite terse and hard-to-read, so I've given a suggestion that I think is faster to understand. It's OK to skip it if you don't like it. src/hotspot/share/oops/instanceKlass.cpp line 187: > 185: if (null_free) return true; > 186: return InlineKlass::cast(this)->is_empty_inline_type(); > 187: } FWIW, I think the following version of the code is easier to understand: Suggestion: bool InstanceKlass::is_naturally_atomic(bool null_free) const { assert(!is_identity_class(), "Doesn't have sense for an identity class"); if (null_free) { // No extra null-marker - just check the layout of the fields _misc_flags.is_naturally_atomic() } else { // Requires a null-marker - can't have any other fields return InlineKlass::cast(this)->is_empty_inline_type(); } } ------------- Marked as reviewed by stefank (Committer). PR Review: https://git.openjdk.org/valhalla/pull/2361#pullrequestreview-4167862976 PR Review Comment: https://git.openjdk.org/valhalla/pull/2361#discussion_r3135472584
