On Wed, 26 Aug 2026 18:25:39 GMT, Stefan Karlsson <[email protected]> wrote:
>> Axel Boldt-Christmas has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update Inline type identity hash comment
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/32535#discussion_r3869581160
