On Tue, 10 Feb 2026 17:40:30 GMT, Frederic Parain <[email protected]> wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove incorrect assert. JVM_CopyOfSpecialArray is currently broken
>
> src/hotspot/share/interpreter/zero/bytecodeInterpreter.cpp line 2681:
>
>> 2679: if (entry->is_flat()) {
>> 2680: CALL_VM(InterpreterRuntime::write_flat_field(THREAD, obj,
>> val, entry), handle_exception);
>> 2681: } else {
>
> The case of null-free arrays was added above, it would be nice to also add
> the support for null-free non-flat fields, even if they are not part of JEP
> 401.
I guess `is_null_free_inline_type` is the correct property here. I did not
realise that `@NullRestricted` was only applicable for fields holding value
types.
> src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 419:
>
>> 417: }
>> 418:
>> 419: inline void FlatValuePayload::copy_from_non_null(BufferedValuePayload&
>> src) {
>
> `copy_from_non_null` looks a bit weird here, because the method does
> something more precise: copying from a buffered value, which implies 1) the
> src is never null and 2) the method is allowed to update the null-marker.
> `copy_from_buffered` looks like a better name to express what the method does.
I did originally call it `copy_from` (buffer is implied by the argument type).
But when I still used the null_reset (null payload) which is a Buffer which is
representing null it was invalid if you called this method. But after removing
using the null_payload it can probably be renamed back `copy_from`.
Also your comment made me realise that the use in `FlatArrayKlass::copy_array`
was wrong. It should not have called `copy_from_non_null` if the `copy_to` call
failed. Pushed a fix and renamed the function.
Will also look at removing the changes w.r.t. null reset value in this change.
We should not really use it in the runtime code. Because trying to use that oop
with `write` or `copy_from` is incorrect. (Might even add an assert for it,
there should only ever be one such object per InlineKlass)
> src/hotspot/share/oops/inlineKlassPayload.inline.hpp line 496:
>
>> 494: InstanceKlass* klass)
>> 495: : FlatFieldPayload(container,
>> 496: klass->field_offset(field_descriptor->index()),
>
> klass->field_offset() is an expensive call, because it decodes the
> FieldInfoStream to get the field offset. The fieldDescriptor argument has a
> cheaper way to get the field offset: field_descriptor->offset().
Right. Sounds good. Just used the same code we already had in the
`jni_[Set|Get]ObjectField`. But we already walked the field stream to find the
field info, let us not walk the field stream again.
-------------
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2792012654
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2791736640
PR Review Comment:
https://git.openjdk.org/valhalla/pull/2068#discussion_r2791820213
