Skip to content
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.

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

8268052: [JVMCI] non-default installed code must be marked as in_use #4283

Closed
wants to merge 1 commit into from
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
@@ -89,7 +89,9 @@ class JVMCI : public AllStatic {
ok,
dependencies_failed,
cache_full,
code_too_large
nmethod_reclaimed, // code cache sweeper reclaimed nmethod in between its creation and being marked "in_use"
code_too_large,
first_permanent_bailout = code_too_large
};

// Gets the handle to the loaded JVMCI shared library, loading it
@@ -479,6 +479,7 @@ JVMCI::CodeInstallResult CodeInstaller::install(JVMCICompiler* compiler,
JVMCIObject target,
JVMCIObject compiled_code,
CodeBlob*& cb,
nmethodLocker& nmethod_handle,
JVMCIObject installed_code,
FailedSpeculation** failed_speculations,
char* speculations,
@@ -536,18 +537,20 @@ JVMCI::CodeInstallResult CodeInstaller::install(JVMCICompiler* compiler,
}

JVMCIObject mirror = installed_code;
nmethod* nm = NULL;
result = runtime()->register_method(jvmci_env(), method, nm, entry_bci, &_offsets, _orig_pc_offset, &buffer,
result = runtime()->register_method(jvmci_env(), method, nmethod_handle, entry_bci, &_offsets, _orig_pc_offset, &buffer,
stack_slots, _debug_recorder->_oopmaps, &_exception_handler_table, &_implicit_exception_table,
compiler, _debug_recorder, _dependencies, id,
has_unsafe_access, _has_wide_vector, compiled_code, mirror,
failed_speculations, speculations, speculations_len);
cb = nm->as_codeblob_or_null();
if (nm != NULL && compile_state == NULL) {
// This compile didn't come through the CompileBroker so perform the printing here
DirectiveSet* directive = DirectivesStack::getMatchingDirective(method, compiler);
nm->maybe_print_nmethod(directive);
DirectivesStack::release(directive);
if (result == JVMCI::ok) {
nmethod* nm = nmethod_handle.code()->as_nmethod_or_null();
cb = nm;
if (compile_state == NULL) {
// This compile didn't come through the CompileBroker so perform the printing here
DirectiveSet* directive = DirectivesStack::getMatchingDirective(method, compiler);
nm->maybe_print_nmethod(directive);
DirectivesStack::release(directive);
}
}
}

@@ -187,6 +187,7 @@ class CodeInstaller : public StackObj {
JVMCIObject target,
JVMCIObject compiled_code,
CodeBlob*& cb,
nmethodLocker& nmethod_handle,
JVMCIObject installed_code,
FailedSpeculation** failed_speculations,
char* speculations,
@@ -881,11 +881,13 @@ C2V_VMENTRY_0(jint, installCode, (JNIEnv *env, jobject, jobject target, jobject

TraceTime install_time("installCode", JVMCICompiler::codeInstallTimer(!thread->is_Compiler_thread()));

nmethodLocker nmethod_handle;
CodeInstaller installer(JVMCIENV);
JVMCI::CodeInstallResult result = installer.install(compiler,
target_handle,
compiled_code_handle,
cb,
nmethod_handle,
installed_code_handle,
(FailedSpeculation**)(address) failed_speculations_address,
speculations,
@@ -1641,7 +1641,7 @@ bool JVMCIRuntime::is_gc_supported(JVMCIEnv* JVMCIENV, CollectedHeap::Name name)
// ------------------------------------------------------------------
JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV,
const methodHandle& method,
nmethod*& nm,
nmethodLocker& code_handle,
int entry_bci,
CodeOffsets* offsets,
int orig_pc_offset,
@@ -1662,7 +1662,7 @@ JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV,
char* speculations,
int speculations_len) {
JVMCI_EXCEPTION_CONTEXT;
nm = NULL;
nmethod* nm = NULL;
int comp_level = CompLevel_full_optimization;
char* failure_detail = NULL;

@@ -1747,6 +1747,7 @@ JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV,
MutexUnlocker locker(MethodCompileQueue_lock);
CompileBroker::handle_full_code_cache(CodeCache::get_code_blob_type(comp_level));
}
result = JVMCI::cache_full;
} else {
nm->set_has_unsafe_access(has_unsafe_access);
nm->set_has_wide_vectors(has_wide_vector);
@@ -1784,6 +1785,8 @@ JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV,
MutexLocker ml(CompiledMethod_lock, Mutex::_no_safepoint_check_flag);
if (nm->make_in_use()) {
method->set_code(method, nm);
} else {
result = JVMCI::nmethod_reclaimed;

This comment has been minimized.

Loading
@tkrodriguez

tkrodriguez Jun 1, 2021
Author Contributor

Is it really possible for the make_in_use call to fail if the nmethod is nmethodLocker'ed? The copy of this logic in ciEnv.cpp doesn't treat a failure of this call to be a compilation failure but it certainly seems like it should. It would be better if it weren't possible for this call to fail.

This comment has been minimized.

Loading
@fisk

fisk Jun 1, 2021
Contributor

The issue is that the nmethod is published to Method code pointers before it is made in_use. That means a concurrent JavaThread may enter it and make the nmethod not_entrant, before it is made in_use. Making it in_use after it became not_entrant would cause a non-monotonic state transition which isn't great and comes with problems.
I agree it feels like there is room for improvement here, but at least that is the reason why make_in_use can fail; to prevent non-monotonic state transitions in this race.

This comment has been minimized.

Loading
@tkrodriguez

tkrodriguez Jun 1, 2021
Author Contributor

Ok. I see the intent. In that case it seems like the reordering of the in_use and set_code calls was the important part of JDK-8226705. It can't actually fail here since it hasn't been published until the following line, right? Or is there some other way the nmethod is published? Why couldn't the in_use state be set at the end of the nmethod factory method?

This comment has been minimized.

Loading
@fisk

fisk Jun 2, 2021
Contributor

I think it's published in the code cache and we had trouble with code walkers using only the CodeCache_lock to shoot down nmethods, but not the Compile_lock. If the walkers used the Compile_lock too then they wouldn't pick these up until they are fully installed. Plenty of room for improvements...

This comment has been minimized.

Loading
@tkrodriguez

tkrodriguez Jun 2, 2021
Author Contributor

Yes, directly scanning the code cache seemed like the only other path. It's unfortunate that the nmethod lifecycle seems to have gotten more complicated instead of less so. So it could really fail in some exceptionally weird circumstances.

This comment has been minimized.

Loading
@fisk

fisk Jun 2, 2021
Contributor

Yes, directly scanning the code cache seemed like the only other path. It's unfortunate that the nmethod lifecycle seems to have gotten more complicated instead of less so. So it could really fail in some exceptionally weird circumstances.

Yes indeed. This is what motivated my work on https://openjdk.java.net/jeps/8221828
In my prototype, after removing inline caches, replacing it with an optimized table driven approach to method invocations, I could reduce the nmethod lifecycle down to a boolean: is_in_use(), which is true or false. No zombies, no unloaded, no unloading, no is_alive, no transient installation state, no separate "nmethod locking mechanism". I hope it hits mainline one day, because it reduces so much risk in our code base, by removing a huge amount of state machinery.

}
} else {
LogTarget(Info, nmethod, install) lt;
@@ -1796,13 +1799,20 @@ JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV,
MutexLocker ml(CompiledMethod_lock, Mutex::_no_safepoint_check_flag);
if (nm->make_in_use()) {
InstanceKlass::cast(method->method_holder())->add_osr_nmethod(nm);
} else {
result = JVMCI::nmethod_reclaimed;
}
}
} else {
assert(!nmethod_mirror.is_hotspot() || data->get_nmethod_mirror(nm, /* phantom_ref */ false) == HotSpotJVMCI::resolve(nmethod_mirror), "must be");
if (!nm->make_in_use()) {
result = JVMCI::nmethod_reclaimed;
}
}
}
result = nm != NULL ? JVMCI::ok :JVMCI::cache_full;
}
if (result == JVMCI::ok) {
code_handle.set_code(nm);
}
}

@@ -1813,8 +1823,8 @@ JVMCI::CodeInstallResult JVMCIRuntime::register_method(JVMCIEnv* JVMCIENV,
JVMCIENV->set_HotSpotCompiledNmethod_installationFailureMessage(compiled_code, message);
}

// JVMTI -- compiled method notification (must be done outside lock)
if (nm != NULL) {
if (result == JVMCI::ok) {
// JVMTI -- compiled method notification (must be done outside lock)
nm->post_compiled_method_load_event();
}

@@ -290,7 +290,7 @@ class JVMCIRuntime: public CHeapObj<mtJVMCI> {
// Register the result of a compilation.
JVMCI::CodeInstallResult register_method(JVMCIEnv* JVMCIENV,
const methodHandle& target,
nmethod*& nm,
nmethodLocker& code_handle,
int entry_bci,
CodeOffsets* offsets,
int orig_pc_offset,
@@ -590,10 +590,13 @@
declare_constant(JumpData::taken_off_set) \
declare_constant(JumpData::displacement_off_set) \
\
declare_preprocessor_constant("JVMCI::ok", JVMCI::ok) \
declare_preprocessor_constant("JVMCI::dependencies_failed", JVMCI::dependencies_failed) \
declare_preprocessor_constant("JVMCI::cache_full", JVMCI::cache_full) \
declare_preprocessor_constant("JVMCI::code_too_large", JVMCI::code_too_large) \
declare_preprocessor_constant("JVMCI::ok", JVMCI::ok) \
declare_preprocessor_constant("JVMCI::dependencies_failed", JVMCI::dependencies_failed) \
declare_preprocessor_constant("JVMCI::cache_full", JVMCI::cache_full) \
declare_preprocessor_constant("JVMCI::code_too_large", JVMCI::code_too_large) \
declare_preprocessor_constant("JVMCI::nmethod_reclaimed", JVMCI::nmethod_reclaimed) \
declare_preprocessor_constant("JVMCI::first_permanent_bailout", JVMCI::first_permanent_bailout) \
\
declare_constant(JVMCIRuntime::none) \
declare_constant(JVMCIRuntime::by_holder) \
declare_constant(JVMCIRuntime::by_full_signature) \
@@ -144,7 +144,7 @@ public InstalledCode installCode(ResolvedJavaMethod method, CompiledCode compile
} else {
msg = String.format("Code installation failed: %s", resultDesc);
}
throw new BailoutException(result != config.codeInstallResultDependenciesFailed, msg);
throw new BailoutException(result >= config.codeInstallResultFirstPermanentBailout, msg);
} else {
throw new BailoutException("Error installing %s: %s", ((HotSpotCompiledCode) compiledCode).getName(), resultDesc);
}
@@ -362,6 +362,8 @@ final int baseVtableLength() {
final int codeInstallResultDependenciesFailed = getConstant("JVMCI::dependencies_failed", Integer.class);
final int codeInstallResultCacheFull = getConstant("JVMCI::cache_full", Integer.class);
final int codeInstallResultCodeTooLarge = getConstant("JVMCI::code_too_large", Integer.class);
final int codeInstallResultNMethodReclaimed = getConstant("JVMCI::nmethod_reclaimed", Integer.class);
final int codeInstallResultFirstPermanentBailout = getConstant("JVMCI::first_permanent_bailout", Integer.class);

String getCodeInstallResultDescription(int codeInstallResult) {
if (codeInstallResult == codeInstallResultOk) {
@@ -376,6 +378,9 @@ String getCodeInstallResultDescription(int codeInstallResult) {
if (codeInstallResult == codeInstallResultCodeTooLarge) {
return "code is too large";
}
if (codeInstallResult == codeInstallResultNMethodReclaimed) {
return "nmethod reclaimed";
}
assert false : codeInstallResult;
return "unknown";
}