Skip to content

8371178: Preserve fast version of getfield and putfield in AOTCache #28121

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ashu-mehra wants to merge 2 commits into from
Closed

8371178: Preserve fast version of getfield and putfield in AOTCache #28121

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5fd053
8371178: Preserve fast version of getfield and putfield in AOTCache
ashu-mehra Oct 24, 2025
40430e3
Address review comments
ashu-mehra Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 120 additions & 7 deletions src/hotspot/share/cds/aotMetaspace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,11 +77,13 @@
#include "memory/universe.hpp"
#include "nmt/memTracker.hpp"
#include "oops/compressedKlass.hpp"
#include "oops/constantPool.inline.hpp"
#include "oops/instanceMirrorKlass.hpp"
#include "oops/klass.inline.hpp"
#include "oops/objArrayOop.hpp"
#include "oops/oop.inline.hpp"
#include "oops/oopHandle.hpp"
#include "oops/resolvedFieldEntry.hpp"
#include "oops/trainingData.hpp"
#include "prims/jvmtiExport.hpp"
#include "runtime/arguments.hpp"
Expand Down Expand Up @@ -520,20 +522,131 @@ void AOTMetaspace::serialize(SerializeClosure* soc) {
}

static void rewrite_nofast_bytecode(const methodHandle& method) {
ConstantPool* cp = method->constants();
BytecodeStream bcs(method);
Bytecodes::Code new_code;

LogStreamHandle(Trace, aot, resolve) lsh;
if (lsh.is_enabled()) {
lsh.print("Rewriting bytecodes for ");
method()->print_external_name(&lsh);
lsh.print("\n");
}

while (!bcs.is_last_bytecode()) {
Bytecodes::Code opcode = bcs.next();
switch (opcode) {
case Bytecodes::_getfield: *bcs.bcp() = Bytecodes::_nofast_getfield; break;
case Bytecodes::_putfield: *bcs.bcp() = Bytecodes::_nofast_putfield; break;
case Bytecodes::_aload_0: *bcs.bcp() = Bytecodes::_nofast_aload_0; break;
case Bytecodes::_iload: {
// Use current opcode as the default value of new_code
new_code = opcode;
switch(opcode) {
case Bytecodes::_getfield: {
uint rfe_index = bcs.get_index_u2();
bool is_resolved = cp->is_resolved(rfe_index, opcode);
#ifdef ASSERT
if (CDSConfig::is_dumping_preimage_static_archive()) {
assert(!is_resolved, "preimage should not have resolved field references");
}
#endif // ASSERT
if (is_resolved) {
Copy link
Member

@iklam iklam Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the above can be simplified:

if (is_resolved) {
  assert(!CDSConfig::is_dumping_preimage_static_archive(), "preimage should not have resolved field references");

Copy link
Contributor Author

@ashu-mehra ashu-mehra Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

ResolvedFieldEntry* rfe = cp->resolved_field_entry_at(bcs.get_index_u2());
switch(rfe->tos_state()) {
case btos:
// fallthrough
case ztos:
new_code = Bytecodes::_fast_bgetfield;
break;
Copy link
Member

@iklam iklam Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this switch block necessary? Is it possible for new_code to be different than *bcs.bcp()? If not, it might be easier to initialize new_code to Bytecodes::_illegal and then do the rewriting only if new_code has been updated.

Copy link
Contributor Author

@ashu-mehra ashu-mehra Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this switch block necessary? Is it possible for new_code to be different than *bcs.bcp()?

During preimage dumping all the fast bytecodes will be converted to nofast version because the CP entries are not resolved. So in the assembly phase *bcs.bcp() would be nofast version. If the CP entry is resolved, we can convert it to fast version. And we determine the type of the fast version using tos_state of the resolved field entry. So the new_code will be different than the *bcs.bcp() in the assembly phase.
They may be same when dumping dynamic archives, but not when dumping static archive or the final AOTCache.
Does that help?

Copy link
Member

@iklam iklam Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Could you add a comment? Maybe the code should be reformatted like this to make it easier to navigate?

        case ftos:  new_code = Bytecodes::_fast_fgetfield;  break;
        case dtos: new_code = Bytecodes::_fast_dgetfield; break;
        default:  ShouldNotReachHere();

Copy link
Contributor Author

@ashu-mehra ashu-mehra Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

case atos:
new_code = Bytecodes::_fast_agetfield;
break;
case itos:
new_code = Bytecodes::_fast_igetfield;
break;
case ctos:
new_code = Bytecodes::_fast_cgetfield;
break;
case stos:
new_code = Bytecodes::_fast_sgetfield;
break;
case ltos:
new_code = Bytecodes::_fast_lgetfield;
break;
case ftos:
new_code = Bytecodes::_fast_fgetfield;
break;
case dtos:
new_code = Bytecodes::_fast_dgetfield;
break;
default:
ShouldNotReachHere();
break;
}
} else {
new_code = Bytecodes::_nofast_getfield;
}
break;
}
case Bytecodes::_putfield: {
uint rfe_index = bcs.get_index_u2();
bool is_resolved = cp->is_resolved(rfe_index, opcode);
#ifdef ASSERT
if (CDSConfig::is_dumping_preimage_static_archive()) {
assert(!is_resolved, "preimage should not have resolved field references");
}
#endif // ASSERT
if (is_resolved) {
ResolvedFieldEntry* rfe = cp->resolved_field_entry_at(bcs.get_index_u2());
switch(rfe->tos_state()) {
case btos:
new_code = Bytecodes::_fast_bputfield;
break;
case ztos:
new_code = Bytecodes::_fast_zputfield;
break;
case atos:
new_code = Bytecodes::_fast_aputfield;
break;
case itos:
new_code = Bytecodes::_fast_iputfield;
break;
case ctos:
new_code = Bytecodes::_fast_cputfield;
break;
case stos:
new_code = Bytecodes::_fast_sputfield;
break;
case ltos:
new_code = Bytecodes::_fast_lputfield;
break;
case ftos:
new_code = Bytecodes::_fast_fputfield;
break;
case dtos:
new_code = Bytecodes::_fast_dputfield;
break;
default:
ShouldNotReachHere();
break;
}
} else {
new_code = Bytecodes::_nofast_putfield;
}
break;
}
case Bytecodes::_aload_0:
new_code = Bytecodes::_nofast_aload_0;
Comment on lines 603 to 605
Copy link
Member

@iklam iklam Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment here that _fast_Xaccess_0 bytecodes will be reverted?

break;
case Bytecodes::_iload:
if (!bcs.is_wide()) {
*bcs.bcp() = Bytecodes::_nofast_iload;
new_code = Bytecodes::_nofast_iload;
}
break;
default:
break;
}
default: break;
if (opcode != new_code) {
*bcs.bcp() = new_code;
if (lsh.is_enabled()) {
lsh.print_cr("%d:%s -> %s", bcs.bci(), Bytecodes::name(opcode), Bytecodes::name(new_code));
}
}
}
}
Expand Down