RFR: 8301996: Move field resolution information out of the cpCache [v2]

Wed Jul 5 18:24:11 UTC 2023

On Thu, 29 Jun 2023 15:44:27 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

>> 8301996: Move field resolution information out of the cpCache
>
> Matias Saavedra Silva has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Coleen and Amit comments

Thank you for tackling this big refactoring work of the cpCache.
I've only reviewed the x86 and the shared part of this PR.
There're several issues to be addressed before this work can move forward.
Feel free to contact me if you have questions about my comments.

src/hotspot/cpu/aarch64/templateTable_aarch64.cpp line 2686:

> 2684: 
> 2685:   Label notVolatile;
> 2686:   __ tbz(raw_flags, 0, notVolatile);

Hard coded values should be avoided. The volatile bit shift should be declared as a constant in ResolvedFieldEntry and used here.

src/hotspot/cpu/x86/templateTable_x86.cpp line 2943:

> 2941:   load_resolved_field_entry(obj, cache, index, off, flags, is_static);
> 2942:   // Index holds the TOS
> 2943:   __ mov(flags, index);

Names are confusing.

src/hotspot/cpu/x86/templateTable_x86.cpp line 2954:

> 2952:   // Make sure we don't need to mask edx after the above shift
> 2953:   assert(btos == 0, "change code, btos != 0");
> 2954:   // Compare the method to zero

Wrong comment.

src/hotspot/cpu/x86/templateTable_x86.cpp line 3187:

> 3185: 
> 3186:   // Swap registers so flags has TOS state
> 3187:   __ xchgl(index, flags);

I don't get this swap operation. Line 3178, index (containing the TosState) has been copied to flags (rax). The real flags for this field have been erased, and the volatile test below is not applied to the real flags but to the ToSState value.

src/hotspot/cpu/x86/templateTable_x86.cpp line 3188:

> 3186:   // Swap registers so flags has TOS state
> 3187:   __ xchgl(index, flags);
> 3188:   __ andl(index, 0x1);

An assert is required here to ensure that the volatile flag is at the least significant bit position.

src/hotspot/cpu/x86/templateTable_x86.cpp line 3452:

> 3450:   load_resolved_field_entry(noreg, cache, rax, rbx, rdx);
> 3451:   // RBX: field offset, RCX: RAX: TOS, RDX: flags
> 3452:   __ andl(rdx, 0x1); //is_volatile

An assert is required here to ensure that the volatile flag is at the least significant bit position.

src/hotspot/share/interpreter/bytecodeTracer.cpp line 328:

> 326:   ConstantPool* constants = _current_method->constants();
> 327:   if (constants->cache() == nullptr) {
> 328:     cp_index = i; // TODO: This is wrong on little-endian. See JDK-8309811.

Fix the code or the comment.

src/hotspot/share/interpreter/bytecodeTracer.cpp line 627:

> 625:     case Bytecodes::_putfield:
> 626:     case Bytecodes::_getfield:
> 627:       // TODO: get_index_u2() does not work here due to using java_u2 instead of native_u2

Is the TODO comment still relevant?

src/hotspot/share/interpreter/interpreterRuntime.cpp line 719:

> 717:   ResolvedFieldEntry* entry = pool->resolved_field_entry_at(field_index);
> 718:   entry->fill_in(info.field_holder(), info.offset(), (u2)info.index(), (u1)state, (u1)get_code, (u1)put_code);
> 719:   entry->set_flags(info.access_flags().is_final(), info.access_flags().is_volatile());

I think there's an issue here: fill_in() will mark the field entry as resolved (by setting get_code or put_code), and by consequence usable by other threads *before* the flags are initialized. This means another thread could see the entry as being resolved but still get the uninitialized flags.

src/hotspot/share/interpreter/interpreterRuntime.cpp line 1178:

> 1176:     h_obj = Handle(current, obj);
> 1177:   }
> 1178:   InstanceKlass* entry_f1 = entry->field_holder();

This variable could have a more explicit name (ik, klass or field_holder)

src/hotspot/share/interpreter/rewriter.cpp line 54:

> 52:     switch (tag) {
> 53:       case JVM_CONSTANT_InterfaceMethodref:
> 54:         add_cp_cache_entry(i);

InterfaceMethodref and Methodref cases could be grouped together.

src/hotspot/share/oops/cpCache.cpp line 746:

> 744:       MetadataFactory::free_array<ResolvedIndyEntry>(data, _resolved_indy_entries);
> 745:     if (_resolved_field_entries)
> 746:       MetadataFactory::free_array<ResolvedFieldEntry>(data, _resolved_field_entries);

Both _resolved_indy_entries and _resolved_field_entries should be set to nullptr.

src/hotspot/share/oops/resolvedFieldEntry.hpp line 73:

> 71:   // Note: Only two flags exists at the moment but more could be added
> 72:   enum {
> 73:       is_final_shift        = 1, // unused

It would be more readable to declare all fields here:
enum {
    is_volatile_ shift                       = 0,
    is_final_shift                             = 1,
};
And using those shift values consistently in setters/getters, even if the current value is zero, would prevent bugs if those flags are re-ordered later.

src/hotspot/share/oops/resolvedFieldEntry.hpp line 95:

> 93:       return (put_code() == code);
> 94:     default:
> 95:       assert(code == Bytecodes::_nop, "Must be get, put, or nop bytecode");

In which cases the is_resolved() method is called with the _nop bytecode in argument?

-------------

Changes requested by fparain (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14129#pullrequestreview-1507211765
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1247960640
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253436948
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253436652
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253464804
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253459999
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253467116
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253270694
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1247917510
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1248168064
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1248198237
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1248202649
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1253292341
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1247874434
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1247894933