On Thu, 15 Jan 2026 13:53:59 GMT, Frederic Parain <[email protected]> wrote:
>> Hello, >> >> Right now we check the null-marker for the NULLABLE_ATOMIC_FLAT LayoutKind >> twice, both before and after allocating a heap instance. The first check >> returns nullptr early if the null-marker is set, as we don't have to >> allocate an object on the heap if we are going to return nullptr anyways. >> The other check is redundant, since if the null-marker is not set, the >> source object is non-null, which means the destination (res) object is >> non-null as well after copying, so the code inside it is unreachable. >> >> Testing: >> * Oracle's tier1-4, hotspot_valhalla, jdk_valhalla >> * I also added a `gurantee` for the removed code and ran through >> hotspot_valhalla and jdk_valhalla locally. > > The second check is needed because the source value might have been mutated > between the first null-marker check and the value copying, and we don't want > to have heap allocated instances being equivalent to a 'null' value. > Here's the scenario: > - thread 1 starts to read a nullable flat value, it checks that the value > is not marked as null, then proceeds to copy the value but is preempted > - thread 2 updates the nullable flat value by writing 'null' to it, which > means setting the null-marker to zero, but also resetting all the other fields > - thread 1 resumes its read, allocate the heap instance, and copy the > payload > > Now, the problem is that there's a heap allocated instance with a payload > that has been reset, which means fields contents might not be valid for this > class. We don't want to have to check the null-marker each time we access a > heap buffered value, we want to preserve the invariant that all heap buffered > values are valid values for the class. So, the null marker is checked a > second time in the heap allocated instance, and if it is zeroed, meaning the > payload written to the instance is equivalent to null, then the instance is > discarded. @fparain I see, thank you for the example and explanation! As this was not immediately clear to me (and others), maybe I can move the direction of this PR to add a brief comment explaining why the additional null-marker check in the end is needed here? ------------- PR Comment: https://git.openjdk.org/valhalla/pull/1914#issuecomment-3755264785
