On Thu, 27 Aug 2026 07:23:07 GMT, Axel Boldt-Christmas <[email protected]>
wrote:
>> src/hotspot/share/cds/aotMappedHeapWriter.cpp line 745:
>>
>>> 743: // We need to retain the identity_hash, because it may have been
>>> used by some hashtables
>>> 744: // in the shared heap.
>>> 745: if (src_obj->has_identity_hash() &&
>>> (!(Arguments::is_valhalla_enabled() && src_obj->mark().is_inline_type()))) {
>>
>> I think this code reads a bit funny. I would prefer if we didn't call
>> `has_identity_has` on identityless objects (value objects). I wonder if this
>> can be turned around to be:
>>
>> Suggestion:
>>
>> if (!src_obj->is_inline_type() && src_obj->has_identity_hash()) {
>>
>> or even if we could have an identity-check and:
>>
>> Suggestion:
>>
>> if (src_obj->has_identity() && !src_obj->has_identity_hash()) {
>
> I find this function a bit strange, and unclear in what its expectations are.
>
> I would like to rewrite it such that it always writes the whole header in the
> fake_oop.
>
> Not sure if the buffer contains unitinitialiaed values, or is 0 initialised.
> But right now we have:
> * `src_obj == nullptr`
> * `+COH`: Whole header created
> * `-COH`: Only klass updated, markWord is not written
> * `src_obj != nullptr`
> * `+COH`: Whole header created
> * `-COH`:
> * `-ValueObject`:
> * `+Identity Hash`: Whole header created preserving hash
> * `-Identity Hash`: Buffer markWord used with age bits cleared
> * _What did this `fake_oop->mark()` contain? All zeros? So fast
> locked, or is there some other step which made this a prototype header_
> * `+ValueObject`: Same as the `-Identity Hash` case
>
> I'll try to investigate this a bit. But probably want to fix it in a bug fix
> if it is the case that this is broken. If it is not broken because something
> else fixes up these oops, I would still like to rewrite this to always write
> the whole header and only write it once and not use the fake_oop as a
> temporary value storage.
>
> I will take your first suggested change in this PR though. The `has_identity`
> is not something that exists right now I think, but is probably a nice
> property to be able to ask.
Right this is the mapped archive case so it should be the original header, I
guess we should not archive locked objects, but can we archive objects with
inflated monitors? Probably is not a bug today, but should probably clean this
up.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3869622167
