RFR: 8301996: Move field resolution information out of the cpCache

[Coleen Phillimore]

On Wed, 24 May 2023 16:55:47 GMT, Matias Saavedra Silva <matsaave at openjdk.org> wrote:

> 8301996: Move field resolution information out of the cpCache

This looks really good!  I have a couple of very minor comments but the rewriter thing I noted should be fixed.

src/hotspot/cpu/x86/interp_masm_x86.cpp line 2132:

> 2130:     shll(index, log2i_exact(sizeof(ResolvedFieldEntry))); // Scale index by power of 2
> 2131:   } else {
> 2132:     imull(index, index, sizeof(ResolvedFieldEntry)); // Scale the index to be the entry index * sizeof(ResolvedInvokeDynamicInfo)

@offamitkumar already noticed this in aarch64, but the comment should be ResolvedFieldEntry
Also, ResolvedInvokeDynamicInfo name was changed to ResolvedIndyEntry so you should fix this in the comment for the indy code.

src/hotspot/share/interpreter/interpreterRuntime.hpp line 123:

> 121:   // Debugger support
> 122:   static void post_field_access(JavaThread* current, oopDesc* obj,
> 123:     ResolvedFieldEntry *entry);

Nit, the star should be with the type: ResolvedFieldEntry* entry.

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

> 189:     int field_entry_index = _cp_map.at(cp_index);
> 190:     Bytes::put_native_u2(p, field_entry_index);
> 191:     if (!_method_handle_invokers.is_empty())

You don't need this code to call maybe_rewrite_invokehandle for fields.

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

> 196:     Bytes::put_Java_u2(p, pool_index);
> 197:     if (!_method_handle_invokers.is_empty())
> 198:       maybe_rewrite_invokehandle(p - 1, pool_index, -1, reverse);

also this code not needed.

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

> 645: static Array<T>* initialize_resolved_entries_array(ClassLoaderData* loader_data, GrowableArray<T> entries, TRAPS) {
> 646:   Array<T>* resolved_entries;
> 647:   if (entries.length()) {

This should this be entries.length() != 0

src/hotspot/share/oops/resolvedFieldEntry.cpp line 42:

> 40:   st->print_cr(" - Is Final: %d", is_final());
> 41:   st->print_cr(" - Is Volatile: %d", is_volatile());
> 42:   st->print_cr(" - Bytecode 1: %s", Bytecodes::name((Bytecodes::Code)get_code()));

should this print Get Code/Put Code instead of Bytecode 1 and 2?

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

> 26: #define SHARE_OOPS_RESOLVEDFIELDENTRY_HPP
> 27: 
> 28: #include "code/compressedStream.hpp"

I don't think this include file is needed.

src/hotspot/share/prims/jvmtiClassFileReconstituter.cpp line 1031:

> 1029:       case Bytecodes::_putfield        :  {
> 1030:         int field_index = Bytes::get_native_u2(bcp+1);
> 1031:         int pool_index = mh->constants()->resolved_field_entry_at(field_index)->constant_pool_index();

This can be declared as a u2 so you don't need the cast below.

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

Changes requested by coleenp (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14129#pullrequestreview-1502091579
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244553656
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244557675
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244559609
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244559776
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244561469
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244563600
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244564667
PR Review Comment: https://git.openjdk.org/jdk/pull/14129#discussion_r1244565139