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

8276658: Clean up JNI local handles code #6336

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
2 changes: 1 addition & 1 deletion src/hotspot/share/ci/ciInstanceKlass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ ciInstanceKlass::ciInstanceKlass(Klass* k) :
(void)CURRENT_ENV->get_object(holder);
}

Thread *thread = Thread::current();
JavaThread *thread = JavaThread::current();
if (ciObjectFactory::is_initialized()) {
_loader = JNIHandles::make_local(thread, ik->class_loader());
_protection_domain = JNIHandles::make_local(thread,
Expand Down
37 changes: 1 addition & 36 deletions src/hotspot/share/compiler/compileBroker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2200,7 +2200,7 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
}

// Allocate a new set of JNI handles.
push_jni_handle_block();
JNIHandleMark jhm(thread);
Method* target_handle = task->method();
int compilable = ciEnv::MethodCompilable;
const char* failure_reason = NULL;
Expand Down Expand Up @@ -2319,9 +2319,6 @@ void CompileBroker::invoke_compiler_on_method(CompileTask* task) {
post_compilation_event(event, task);
}
}
// Remove the JNI handle block after the ciEnv destructor has run in
// the previous block.
pop_jni_handle_block();
coleenp marked this conversation as resolved.
Show resolved Hide resolved

if (failure_reason != NULL) {
task->set_failure_reason(failure_reason, failure_reason_on_C_heap);
Expand Down Expand Up @@ -2482,38 +2479,6 @@ void CompileBroker::update_compile_perf_data(CompilerThread* thread, const metho
counters->set_compile_type((jlong) last_compile_type);
}

// ------------------------------------------------------------------
// CompileBroker::push_jni_handle_block
//
// Push on a new block of JNI handles.
void CompileBroker::push_jni_handle_block() {
JavaThread* thread = JavaThread::current();

// Allocate a new block for JNI handles.
// Inlined code from jni_PushLocalFrame()
JNIHandleBlock* java_handles = thread->active_handles();
JNIHandleBlock* compile_handles = JNIHandleBlock::allocate_block(thread);
assert(compile_handles != NULL && java_handles != NULL, "should not be NULL");
compile_handles->set_pop_frame_link(java_handles); // make sure java handles get gc'd.
thread->set_active_handles(compile_handles);
}


// ------------------------------------------------------------------
// CompileBroker::pop_jni_handle_block
//
// Pop off the current block of JNI handles.
void CompileBroker::pop_jni_handle_block() {
JavaThread* thread = JavaThread::current();

// Release our JNI handle block
JNIHandleBlock* compile_handles = thread->active_handles();
JNIHandleBlock* java_handles = compile_handles->pop_frame_link();
thread->set_active_handles(java_handles);
compile_handles->set_pop_frame_link(NULL);
JNIHandleBlock::release_block(compile_handles, thread); // may block
}

// ------------------------------------------------------------------
// CompileBroker::collect_statistics
//
Expand Down
2 changes: 0 additions & 2 deletions src/hotspot/share/compiler/compileBroker.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -259,8 +259,6 @@ class CompileBroker: AllStatic {
int compilable, const char* failure_reason);
static void update_compile_perf_data(CompilerThread *thread, const methodHandle& method, bool is_osr);

static void push_jni_handle_block();
static void pop_jni_handle_block();
static void collect_statistics(CompilerThread* thread, elapsedTimer time, CompileTask* task);

static void compile_method_base(const methodHandle& method,
Expand Down
5 changes: 1 addition & 4 deletions src/hotspot/share/gc/shared/concurrentGCThread.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2001, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2001, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -42,9 +42,6 @@ void ConcurrentGCThread::create_and_start(ThreadPriority prio) {
}

void ConcurrentGCThread::run() {
// Setup handle area
set_active_handles(JNIHandleBlock::allocate_block());

// Wait for initialization to complete
wait_init_completed();

Expand Down
53 changes: 4 additions & 49 deletions src/hotspot/share/jfr/dcmd/jfrDcmds.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -55,53 +55,6 @@ bool register_jfr_dcmds() {
return true;
}

// JNIHandle management

// ------------------------------------------------------------------
// push_jni_handle_block
//
// Push on a new block of JNI handles.
static void push_jni_handle_block(JavaThread* const thread) {
DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(thread));

// Allocate a new block for JNI handles.
// Inlined code from jni_PushLocalFrame()
JNIHandleBlock* prev_handles = thread->active_handles();
JNIHandleBlock* entry_handles = JNIHandleBlock::allocate_block(thread);
assert(entry_handles != NULL && prev_handles != NULL, "should not be NULL");
entry_handles->set_pop_frame_link(prev_handles); // make sure prev handles get gc'd.
thread->set_active_handles(entry_handles);
}

// ------------------------------------------------------------------
// pop_jni_handle_block
//
// Pop off the current block of JNI handles.
static void pop_jni_handle_block(JavaThread* const thread) {
DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(thread));

// Release our JNI handle block
JNIHandleBlock* entry_handles = thread->active_handles();
JNIHandleBlock* prev_handles = entry_handles->pop_frame_link();
// restore
thread->set_active_handles(prev_handles);
entry_handles->set_pop_frame_link(NULL);
JNIHandleBlock::release_block(entry_handles, thread); // may block
}

class JNIHandleBlockManager : public StackObj {
private:
JavaThread* const _thread;
public:
JNIHandleBlockManager(JavaThread* thread) : _thread(thread) {
push_jni_handle_block(_thread);
}

~JNIHandleBlockManager() {
pop_jni_handle_block(_thread);
}
};

static bool is_module_available(outputStream* output, TRAPS) {
return JfrJavaSupport::is_jdk_jfr_module_available(output, THREAD);
}
Expand Down Expand Up @@ -223,7 +176,9 @@ void JfrDCmd::invoke(JfrJavaArguments& method, TRAPS) const {
constructor_args.set_klass(javaClass(), CHECK);

HandleMark hm(THREAD);
JNIHandleBlockManager jni_handle_management(THREAD);
JNIHandleMark jni_handle_management(THREAD);

DEBUG_ONLY(JfrJavaSupport::check_java_thread_in_vm(THREAD));
Copy link
Contributor

Choose a reason for hiding this comment

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

This method will call into Java below which already checks the thread is in vm so maybe this is not necessary. Even construct_dcmd_instance() has that assert.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it's doubly redundant. I'll remove it.


const oop dcmd = construct_dcmd_instance(&constructor_args, CHECK);

Expand Down Expand Up @@ -494,7 +449,7 @@ void JfrConfigureFlightRecorderDCmd::execute(DCmdSource source, TRAPS) {
}

HandleMark hm(THREAD);
JNIHandleBlockManager jni_handle_management(THREAD);
JNIHandleMark jni_handle_management(THREAD);

JavaValue result(T_OBJECT);
JfrJavaArguments constructor_args(&result);
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/jfr/jni/jfrJavaSupport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ static void check_new_unstarted_java_thread(JavaThread* t) {
*/
jobject JfrJavaSupport::local_jni_handle(const oop obj, JavaThread* t) {
DEBUG_ONLY(check_java_thread_in_vm(t));
return t->active_handles()->allocate_handle(obj);
return t->active_handles()->allocate_handle(t, obj);
}

jobject JfrJavaSupport::local_jni_handle(const jobject handle, JavaThread* t) {
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/jvmci/jvmciCodeInstaller.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -899,7 +899,7 @@ GrowableArray<ScopeValue*>* CodeInstaller::record_virtual_objects(JVMCIObject de
}
Klass* klass = jvmci_env()->asKlass(type);
oop javaMirror = klass->java_mirror();
ScopeValue *klass_sv = new ConstantOopWriteValue(JNIHandles::make_local(Thread::current(), javaMirror));
ScopeValue *klass_sv = new ConstantOopWriteValue(JNIHandles::make_local(javaMirror));
ObjectValue* sv = is_auto_box ? new AutoBoxObjectValue(id, klass_sv) : new ObjectValue(id, klass_sv);
if (id < 0 || id >= objects->length()) {
JVMCI_ERROR_NULL("virtual object id %d out of bounds", id);
Expand Down
23 changes: 0 additions & 23 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,29 +86,6 @@ static void requireInHotSpot(const char* caller, JVMCI_TRAPS) {
}
}

void JNIHandleMark::push_jni_handle_block(JavaThread* thread) {
if (thread != NULL) {
// Allocate a new block for JNI handles.
// Inlined code from jni_PushLocalFrame()
JNIHandleBlock* java_handles = thread->active_handles();
JNIHandleBlock* compile_handles = JNIHandleBlock::allocate_block(thread);
assert(compile_handles != NULL && java_handles != NULL, "should not be NULL");
compile_handles->set_pop_frame_link(java_handles);
thread->set_active_handles(compile_handles);
}
}

void JNIHandleMark::pop_jni_handle_block(JavaThread* thread) {
if (thread != NULL) {
// Release our JNI handle block
JNIHandleBlock* compile_handles = thread->active_handles();
JNIHandleBlock* java_handles = compile_handles->pop_frame_link();
thread->set_active_handles(java_handles);
compile_handles->set_pop_frame_link(NULL);
JNIHandleBlock::release_block(compile_handles, thread); // may block
}
}

class JVMCITraceMark : public StackObj {
const char* _msg;
public:
Expand Down
13 changes: 1 addition & 12 deletions src/hotspot/share/jvmci/jvmciCompilerToVM.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2011, 2021, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -173,15 +173,4 @@ class JavaArgumentUnboxer : public SignatureIterator {
}
};

class JNIHandleMark : public StackObj {
JavaThread* _thread;
public:
JNIHandleMark(JavaThread* thread) : _thread(thread) { push_jni_handle_block(thread); }
~JNIHandleMark() { pop_jni_handle_block(_thread); }

private:
static void push_jni_handle_block(JavaThread* thread);
static void pop_jni_handle_block(JavaThread* thread);
};

#endif // SHARE_JVMCI_JVMCICOMPILERTOVM_HPP
7 changes: 2 additions & 5 deletions src/hotspot/share/prims/jni.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -639,11 +639,8 @@ JNI_ENTRY(jint, jni_PushLocalFrame(JNIEnv *env, jint capacity))
HOTSPOT_JNI_PUSHLOCALFRAME_RETURN((uint32_t)JNI_ERR);
return JNI_ERR;
}
JNIHandleBlock* old_handles = thread->active_handles();
JNIHandleBlock* new_handles = JNIHandleBlock::allocate_block(thread);
assert(new_handles != NULL, "should not be NULL");
new_handles->set_pop_frame_link(old_handles);
thread->set_active_handles(new_handles);

thread->push_jni_handle_block();
jint ret = JNI_OK;
HOTSPOT_JNI_PUSHLOCALFRAME_RETURN(ret);
return ret;
Expand Down
2 changes: 1 addition & 1 deletion src/hotspot/share/prims/jvmtiEnvBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1510,7 +1510,7 @@ JvmtiModuleClosure::get_all_modules(JvmtiEnv* env, jint* module_count_ptr, jobje
return JVMTI_ERROR_OUT_OF_MEMORY;
}
for (jint idx = 0; idx < len; idx++) {
array[idx] = JNIHandles::make_local(Thread::current(), _tbl->at(idx).resolve());
array[idx] = JNIHandles::make_local(_tbl->at(idx).resolve());
}
_tbl = NULL;
*modules_ptr = array;
Expand Down
37 changes: 2 additions & 35 deletions src/hotspot/share/prims/jvmtiExport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -141,52 +141,25 @@ class JvmtiEventMark : public StackObj {
JavaThread *_thread;
JNIEnv* _jni_env;
JvmtiThreadState::ExceptionState _saved_exception_state;
#if 0
JNIHandleBlock* _hblock;
#endif

public:
JvmtiEventMark(JavaThread *thread) : _thread(thread),
_jni_env(thread->jni_environment()),
_saved_exception_state(JvmtiThreadState::ES_CLEARED) {
#if 0
_hblock = thread->active_handles();
_hblock->clear_thoroughly(); // so we can be safe
#else
// we want to use the code above - but that needs the JNIHandle changes - later...
// for now, steal JNI push local frame code
JvmtiThreadState *state = thread->jvmti_thread_state();
// we are before an event.
// Save current jvmti thread exception state.
if (state != NULL) {
_saved_exception_state = state->get_exception_state();
}

JNIHandleBlock* old_handles = thread->active_handles();
JNIHandleBlock* new_handles = JNIHandleBlock::allocate_block(thread);
assert(new_handles != NULL, "should not be NULL");
new_handles->set_pop_frame_link(old_handles);
thread->set_active_handles(new_handles);
#endif
thread->push_jni_handle_block();
assert(thread == JavaThread::current(), "thread must be current!");
thread->frame_anchor()->make_walkable(thread);
};

~JvmtiEventMark() {
#if 0
_hblock->clear(); // for consistency with future correct behavior
#else
// we want to use the code above - but that needs the JNIHandle changes - later...
// for now, steal JNI pop local frame code
JNIHandleBlock* old_handles = _thread->active_handles();
JNIHandleBlock* new_handles = old_handles->pop_frame_link();
assert(new_handles != NULL, "should not be NULL");
_thread->set_active_handles(new_handles);
// Note that we set the pop_frame_link to NULL explicitly, otherwise
// the release_block call will release the blocks.
old_handles->set_pop_frame_link(NULL);
JNIHandleBlock::release_block(old_handles, _thread); // may block
#endif
_thread->pop_jni_handle_block();

JvmtiThreadState* state = _thread->jvmti_thread_state();
// we are continuing after an event.
Expand All @@ -196,13 +169,7 @@ class JvmtiEventMark : public StackObj {
}
}

#if 0
jobject to_jobject(oop obj) { return obj == NULL? NULL : _hblock->allocate_handle_fast(obj); }
#else
// we want to use the code above - but that needs the JNIHandle changes - later...
// for now, use regular make_local
jobject to_jobject(oop obj) { return JNIHandles::make_local(_thread,obj); }
#endif

jclass to_jclass(Klass* klass) { return (klass == NULL ? NULL : (jclass)to_jobject(klass->java_mirror())); }

Expand Down
Loading