JNI: Faster jobject decoding by avoiding checks.
Avoid costly runtime checks in Thread::DecodeJObject() and
IndirectReferenceTable::Get() that it calls and replace them
with DCHECK()s and checks in CheckJNI. This improves the
performance of JNI interface in release mode for processes
that do not use CheckJNI (default for non-debuggable apps).
The results for StringToBytesBenchmark on blueline little
cores running at fixed frequency 1420800 are approximately
(medians from 3 runs) before after
timeGetBytesAscii EMPTY 477.70 408.18
timeGetBytesIso88591 EMPTY 473.00 412.15
timeGetBytesUtf8 EMPTY 468.96 402.78
Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Test: blueline-userdebug boots.
Bug: 172332525
Change-Id: Ibea788bb54879d1fca0608c30fde008063aaafcc
diff --git a/runtime/indirect_reference_table-inl.h b/runtime/indirect_reference_table-inl.h
index 2128f8c..fb2dc1f 100644
--- a/runtime/indirect_reference_table-inl.h
+++ b/runtime/indirect_reference_table-inl.h
@@ -33,36 +33,28 @@
// Verifies that the indirect table lookup is valid.
// Returns "false" if something looks bad.
-inline bool IndirectReferenceTable::GetChecked(IndirectRef iref) const {
- if (UNLIKELY(iref == nullptr)) {
- LOG(WARNING) << "Attempt to look up nullptr " << kind_;
- return false;
- }
- if (UNLIKELY(GetIndirectRefKind(iref) == kHandleScopeOrInvalid)) {
- AbortIfNoCheckJNI(android::base::StringPrintf("JNI ERROR (app bug): invalid %s %p",
- GetIndirectRefKindString(kind_),
- iref));
- return false;
- }
+inline bool IndirectReferenceTable::IsValidReference(IndirectRef iref,
+ /*out*/std::string* error_msg) const {
+ DCHECK(iref != nullptr);
+ DCHECK_EQ(GetIndirectRefKind(iref), kind_);
const uint32_t top_index = segment_state_.top_index;
uint32_t idx = ExtractIndex(iref);
if (UNLIKELY(idx >= top_index)) {
- std::string msg = android::base::StringPrintf(
- "JNI ERROR (app bug): accessed stale %s %p (index %d in a table of size %d)",
- GetIndirectRefKindString(kind_),
- iref,
- idx,
- top_index);
- AbortIfNoCheckJNI(msg);
+ *error_msg = android::base::StringPrintf("deleted reference at index %u in a table of size %u",
+ idx,
+ top_index);
return false;
}
if (UNLIKELY(table_[idx].GetReference()->IsNull())) {
- AbortIfNoCheckJNI(android::base::StringPrintf("JNI ERROR (app bug): accessed deleted %s %p",
- GetIndirectRefKindString(kind_),
- iref));
+ *error_msg = android::base::StringPrintf("deleted reference at index %u", idx);
return false;
}
- if (UNLIKELY(!CheckEntry("use", iref, idx))) {
+ uint32_t iref_serial = DecodeSerial(reinterpret_cast<uintptr_t>(iref));
+ uint32_t entry_serial = table_[idx].GetSerial();
+ if (UNLIKELY(iref_serial != entry_serial)) {
+ *error_msg = android::base::StringPrintf("stale reference with serial number %u v. current %u",
+ iref_serial,
+ entry_serial);
return false;
}
return true;
@@ -88,21 +80,22 @@
template<ReadBarrierOption kReadBarrierOption>
inline ObjPtr<mirror::Object> IndirectReferenceTable::Get(IndirectRef iref) const {
- if (!GetChecked(iref)) {
- return nullptr;
- }
+ DCHECK_EQ(GetIndirectRefKind(iref), kind_);
uint32_t idx = ExtractIndex(iref);
+ DCHECK_LT(idx, segment_state_.top_index);
+ DCHECK_EQ(DecodeSerial(reinterpret_cast<uintptr_t>(iref)), table_[idx].GetSerial());
+ DCHECK(!table_[idx].GetReference()->IsNull());
ObjPtr<mirror::Object> obj = table_[idx].GetReference()->Read<kReadBarrierOption>();
VerifyObject(obj);
return obj;
}
inline void IndirectReferenceTable::Update(IndirectRef iref, ObjPtr<mirror::Object> obj) {
- if (!GetChecked(iref)) {
- LOG(WARNING) << "IndirectReferenceTable Update failed to find reference " << iref;
- return;
- }
+ DCHECK_EQ(GetIndirectRefKind(iref), kind_);
uint32_t idx = ExtractIndex(iref);
+ DCHECK_LT(idx, segment_state_.top_index);
+ DCHECK_EQ(DecodeSerial(reinterpret_cast<uintptr_t>(iref)), table_[idx].GetSerial());
+ DCHECK(!table_[idx].GetReference()->IsNull());
table_[idx].SetReference(obj);
}
diff --git a/runtime/indirect_reference_table.h b/runtime/indirect_reference_table.h
index 86b92cf..f877ce8 100644
--- a/runtime/indirect_reference_table.h
+++ b/runtime/indirect_reference_table.h
@@ -285,6 +285,10 @@
REQUIRES_SHARED(Locks::mutator_lock_)
REQUIRES(!Locks::alloc_tracker_lock_);
+ IndirectRefKind GetKind() const {
+ return kind_;
+ }
+
// Return the #of entries in the entire table. This includes holes, and
// so may be larger than the actual number of "live" entries.
size_t Capacity() const {
@@ -331,6 +335,10 @@
return DecodeIndirectRefKind(reinterpret_cast<uintptr_t>(iref));
}
+ /* Reference validation for CheckJNI. */
+ bool IsValidReference(IndirectRef, /*out*/std::string* error_msg) const
+ REQUIRES_SHARED(Locks::mutator_lock_);
+
private:
static constexpr size_t kSerialBits = MinimumBitsToStore(kIRTPrevCount);
static constexpr uint32_t kShiftedSerialMask = (1u << kSerialBits) - 1;
@@ -391,7 +399,6 @@
static void AbortIfNoCheckJNI(const std::string& msg);
/* extra debugging checks */
- bool GetChecked(IndirectRef) const REQUIRES_SHARED(Locks::mutator_lock_);
bool CheckEntry(const char*, IndirectRef, uint32_t) const;
/// semi-public - read/write by jni down calls.
diff --git a/runtime/indirect_reference_table_test.cc b/runtime/indirect_reference_table_test.cc
index c5ae4c6..5da7a30 100644
--- a/runtime/indirect_reference_table_test.cc
+++ b/runtime/indirect_reference_table_test.cc
@@ -106,8 +106,9 @@
// Table should be empty now.
EXPECT_EQ(0U, irt.Capacity());
- // Get invalid entry (off the end of the list).
- EXPECT_TRUE(irt.Get(iref0) == nullptr);
+ // Check that the entry off the end of the list is not valid.
+ // (CheckJNI shall abort for such entries.)
+ EXPECT_FALSE(irt.IsValidReference(iref0, &error_msg));
// Add three, remove in the opposite order.
iref0 = irt.Add(cookie, obj0.Get(), &error_msg);
@@ -145,8 +146,8 @@
ASSERT_FALSE(irt.Remove(cookie, iref1));
CheckDump(&irt, 2, 2);
- // Get invalid entry (from hole).
- EXPECT_TRUE(irt.Get(iref1) == nullptr);
+ // Check that the reference to the hole is not valid.
+ EXPECT_FALSE(irt.IsValidReference(iref1, &error_msg));
ASSERT_TRUE(irt.Remove(cookie, iref2));
CheckDump(&irt, 1, 1);
@@ -227,15 +228,12 @@
ASSERT_EQ(0U, irt.Capacity()) << "temporal del not empty";
CheckDump(&irt, 0, 0);
- // null isn't a valid iref.
- ASSERT_TRUE(irt.Get(nullptr) == nullptr);
-
- // Stale lookup.
+ // Stale reference is not valid.
iref0 = irt.Add(cookie, obj0.Get(), &error_msg);
EXPECT_TRUE(iref0 != nullptr);
CheckDump(&irt, 1, 1);
ASSERT_TRUE(irt.Remove(cookie, iref0));
- EXPECT_TRUE(irt.Get(iref0) == nullptr) << "stale lookup succeeded";
+ EXPECT_FALSE(irt.IsValidReference(iref0, &error_msg)) << "stale lookup succeeded";
CheckDump(&irt, 0, 0);
// Test table resizing.
@@ -322,7 +320,7 @@
// Must not have filled the previous hole.
EXPECT_EQ(irt.Capacity(), 4u);
- EXPECT_TRUE(irt.Get(iref1) == nullptr);
+ EXPECT_FALSE(irt.IsValidReference(iref1, &error_msg));
CheckDump(&irt, 3, 3);
UNUSED(iref0, iref1, iref2, iref3);
@@ -357,7 +355,7 @@
IndirectRef iref4 = irt.Add(cookie1, obj4.Get(), &error_msg);
EXPECT_EQ(irt.Capacity(), 2u);
- EXPECT_TRUE(irt.Get(iref2) == nullptr);
+ EXPECT_FALSE(irt.IsValidReference(iref2, &error_msg));
CheckDump(&irt, 2, 2);
UNUSED(iref0, iref1, iref2, iref3, iref4);
@@ -397,7 +395,7 @@
IndirectRef iref4 = irt.Add(cookie1, obj4.Get(), &error_msg);
EXPECT_EQ(irt.Capacity(), 3u);
- EXPECT_TRUE(irt.Get(iref1) == nullptr);
+ EXPECT_FALSE(irt.IsValidReference(iref1, &error_msg));
CheckDump(&irt, 3, 3);
UNUSED(iref0, iref1, iref2, iref3, iref4);
@@ -439,7 +437,7 @@
IndirectRef iref5 = irt.Add(cookie1, obj4.Get(), &error_msg);
EXPECT_EQ(irt.Capacity(), 2u);
- EXPECT_TRUE(irt.Get(iref3) == nullptr);
+ EXPECT_FALSE(irt.IsValidReference(iref3, &error_msg));
CheckDump(&irt, 2, 2);
UNUSED(iref0, iref1, iref2, iref3, iref4, iref5);
@@ -479,7 +477,7 @@
IndirectRef iref4 = irt.Add(cookie1, obj3.Get(), &error_msg);
EXPECT_EQ(irt.Capacity(), 2u);
- EXPECT_TRUE(irt.Get(iref3) == nullptr);
+ EXPECT_FALSE(irt.IsValidReference(iref3, &error_msg));
CheckDump(&irt, 2, 2);
UNUSED(iref0, iref1, iref2, iref3, iref4);
diff --git a/runtime/jni/check_jni.cc b/runtime/jni/check_jni.cc
index 1626357..42e46e9 100644
--- a/runtime/jni/check_jni.cc
+++ b/runtime/jni/check_jni.cc
@@ -35,6 +35,7 @@
#include "dex/descriptors_names.h"
#include "dex/dex_file-inl.h"
#include "gc/space/space.h"
+#include "indirect_reference_table-inl.h"
#include "java_vm_ext.h"
#include "jni_internal.h"
#include "mirror/class-inl.h"
@@ -50,6 +51,20 @@
#include "well_known_classes.h"
namespace art {
+
+// This helper cannot be in the anonymous namespace because it needs to be
+// declared as a friend by JniVmExt and JniEnvExt.
+inline IndirectReferenceTable* GetIndirectReferenceTable(ScopedObjectAccess& soa,
+ IndirectRefKind kind) {
+ DCHECK_NE(kind, kHandleScopeOrInvalid);
+ JNIEnvExt* env = soa.Env();
+ IndirectReferenceTable* irt =
+ (kind == kLocal) ? &env->locals_
+ : ((kind == kGlobal) ? &env->vm_->globals_ : &env->vm_->weak_globals_);
+ DCHECK_EQ(irt->GetKind(), kind);
+ return irt;
+}
+
namespace {
using android::base::StringAppendF;
@@ -842,26 +857,59 @@
}
}
- ObjPtr<mirror::Object> obj = soa.Decode<mirror::Object>(java_object);
- if (obj == nullptr) {
- // Either java_object is invalid or is a cleared weak.
- IndirectRef ref = reinterpret_cast<IndirectRef>(java_object);
- bool okay;
- if (IndirectReferenceTable::GetIndirectRefKind(ref) != kWeakGlobal) {
+ ObjPtr<mirror::Object> obj = nullptr;
+ IndirectRef ref = reinterpret_cast<IndirectRef>(java_object);
+ IndirectRefKind ref_kind = IndirectReferenceTable::GetIndirectRefKind(ref);
+ bool expect_null = false;
+ bool okay = true;
+ std::string error_msg;
+ if (ref_kind == kHandleScopeOrInvalid) {
+ if (!soa.Self()->HandleScopeContains(java_object)) {
okay = false;
+ error_msg = "use of invalid jobject";
} else {
- obj = soa.Vm()->DecodeWeakGlobal(soa.Self(), ref);
- okay = Runtime::Current()->IsClearedJniWeakGlobal(obj);
+ obj = soa.Decode<mirror::Object>(java_object);
}
- if (!okay) {
- AbortF("%s is an invalid %s: %p (%p)",
- what,
- GetIndirectRefKindString(IndirectReferenceTable::GetIndirectRefKind(java_object)),
- java_object,
- obj.Ptr());
- return false;
+ } else {
+ IndirectReferenceTable* irt = GetIndirectReferenceTable(soa, ref_kind);
+ okay = irt->IsValidReference(java_object, &error_msg);
+ DCHECK_EQ(okay, error_msg.empty());
+ if (okay) {
+ // Note: The `IsValidReference()` checks for null but we do not prevent races,
+ // so the null check below can still fail. Even if it succeeds, another thread
+ // could delete the global or weak global before it's used by JNI.
+ if (ref_kind == kLocal) {
+ // Local references do not need a read barrier.
+ obj = irt->Get<kWithoutReadBarrier>(ref);
+ } else if (ref_kind == kGlobal) {
+ obj = soa.Env()->GetVm()->DecodeGlobal(ref);
+ } else {
+ obj = soa.Env()->GetVm()->DecodeWeakGlobal(soa.Self(), ref);
+ if (Runtime::Current()->IsClearedJniWeakGlobal(obj)) {
+ obj = nullptr;
+ expect_null = true;
+ }
+ }
}
}
+ if (okay) {
+ if (!expect_null && obj == nullptr) {
+ okay = false;
+ error_msg = "deleted reference";
+ }
+ if (expect_null && !null_ok) {
+ okay = false;
+ error_msg = "cleared weak reference";
+ }
+ }
+ if (!okay) {
+ AbortF("JNI ERROR (app bug): %s is an invalid %s: %p (%s)",
+ what,
+ ToStr<IndirectRefKind>(ref_kind).c_str(),
+ java_object,
+ error_msg.c_str());
+ return false;
+ }
if (!Runtime::Current()->GetHeap()->IsValidObjectAddress(obj.Ptr())) {
Runtime::Current()->GetHeap()->DumpSpaces(LOG_STREAM(ERROR));
@@ -873,7 +921,6 @@
return false;
}
- bool okay = true;
switch (kind) {
case kClass:
okay = obj->IsClass();
diff --git a/runtime/jni/java_vm_ext.h b/runtime/jni/java_vm_ext.h
index 6f7e546..015f85c 100644
--- a/runtime/jni/java_vm_ext.h
+++ b/runtime/jni/java_vm_ext.h
@@ -37,6 +37,7 @@
class ParsedOptions;
class Runtime;
struct RuntimeArgumentMap;
+class ScopedObjectAccess;
class JavaVMExt;
// Hook definition for runtime plugins.
@@ -263,6 +264,9 @@
std::atomic<bool> allocation_tracking_enabled_;
std::atomic<bool> old_allocation_tracking_state_;
+ friend IndirectReferenceTable* GetIndirectReferenceTable(ScopedObjectAccess& soa,
+ IndirectRefKind kind);
+
DISALLOW_COPY_AND_ASSIGN(JavaVMExt);
};
diff --git a/runtime/jni/jni_env_ext.h b/runtime/jni/jni_env_ext.h
index 5d4304d..2fae8d2 100644
--- a/runtime/jni/jni_env_ext.h
+++ b/runtime/jni/jni_env_ext.h
@@ -30,6 +30,7 @@
class ArtMethod;
class ArtField;
class JavaVMExt;
+class ScopedObjectAccess;
class ScopedObjectAccessAlreadyRunnable;
namespace mirror {
@@ -212,6 +213,8 @@
template<bool kEnableIndexIds> friend class JNI;
friend class ScopedJniEnvLocalRefState;
friend class Thread;
+ friend IndirectReferenceTable* GetIndirectReferenceTable(ScopedObjectAccess& soa,
+ IndirectRefKind kind);
friend void ThreadResetFunctionTable(Thread* thread, void* arg);
ART_FRIEND_TEST(JniInternalTest, JNIEnvExtOffsets);
};
diff --git a/runtime/jni/jni_internal_test.cc b/runtime/jni/jni_internal_test.cc
index a4cfd3a..2793857 100644
--- a/runtime/jni/jni_internal_test.cc
+++ b/runtime/jni/jni_internal_test.cc
@@ -1996,17 +1996,12 @@
ASSERT_NE(s, nullptr);
env_->DeleteLocalRef(s);
- // Currently, deleting an already-deleted reference is just a CheckJNI warning.
+ // Currently, deleting an already-deleted reference is just a CheckJNI abort.
{
- bool old_check_jni = vm_->SetCheckJniEnabled(false);
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
- env_->DeleteLocalRef(s);
- }
+ bool old_check_jni = vm_->SetCheckJniEnabled(true);
CheckJniAbortCatcher check_jni_abort_catcher;
- EXPECT_FALSE(vm_->SetCheckJniEnabled(true));
env_->DeleteLocalRef(s);
- std::string expected(StringPrintf("use of deleted local reference %p", s));
+ std::string expected = StringPrintf("jobject is an invalid local reference: %p", s);
check_jni_abort_catcher.Check(expected.c_str());
EXPECT_TRUE(vm_->SetCheckJniEnabled(old_check_jni));
}
@@ -2061,7 +2056,7 @@
{
CheckJniAbortCatcher check_jni_abort_catcher;
EXPECT_EQ(JNIInvalidRefType, env_->GetObjectRefType(inner1));
- check_jni_abort_catcher.Check("use of deleted local reference");
+ check_jni_abort_catcher.Check("jobject is an invalid local reference");
}
// Our local reference for the survivor is invalid because the survivor
@@ -2069,7 +2064,7 @@
{
CheckJniAbortCatcher check_jni_abort_catcher;
EXPECT_EQ(JNIInvalidRefType, env_->GetObjectRefType(inner2));
- check_jni_abort_catcher.Check("use of deleted local reference");
+ check_jni_abort_catcher.Check("jobject is an invalid local reference");
}
EXPECT_EQ(env_->PopLocalFrame(nullptr), nullptr);
@@ -2077,11 +2072,11 @@
EXPECT_EQ(JNILocalRefType, env_->GetObjectRefType(original));
CheckJniAbortCatcher check_jni_abort_catcher;
EXPECT_EQ(JNIInvalidRefType, env_->GetObjectRefType(outer));
- check_jni_abort_catcher.Check("use of deleted local reference");
+ check_jni_abort_catcher.Check("jobject is an invalid local reference");
EXPECT_EQ(JNIInvalidRefType, env_->GetObjectRefType(inner1));
- check_jni_abort_catcher.Check("use of deleted local reference");
+ check_jni_abort_catcher.Check("jobject is an invalid local reference");
EXPECT_EQ(JNIInvalidRefType, env_->GetObjectRefType(inner2));
- check_jni_abort_catcher.Check("use of deleted local reference");
+ check_jni_abort_catcher.Check("jobject is an invalid local reference");
}
TEST_F(JniInternalTest, PushLocalFrame_LimitAndOverflow) {
@@ -2135,17 +2130,12 @@
ASSERT_NE(o, nullptr);
env_->DeleteGlobalRef(o);
- // Currently, deleting an already-deleted reference is just a CheckJNI warning.
+ // Currently, deleting an already-deleted reference is just a CheckJNI abort.
{
- bool old_check_jni = vm_->SetCheckJniEnabled(false);
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
- env_->DeleteGlobalRef(o);
- }
+ bool old_check_jni = vm_->SetCheckJniEnabled(true);
CheckJniAbortCatcher check_jni_abort_catcher;
- EXPECT_FALSE(vm_->SetCheckJniEnabled(true));
env_->DeleteGlobalRef(o);
- std::string expected(StringPrintf("use of deleted global reference %p", o));
+ std::string expected = StringPrintf("jobject is an invalid global reference: %p", o);
check_jni_abort_catcher.Check(expected.c_str());
EXPECT_TRUE(vm_->SetCheckJniEnabled(old_check_jni));
}
@@ -2188,17 +2178,12 @@
ASSERT_NE(o, nullptr);
env_->DeleteWeakGlobalRef(o);
- // Currently, deleting an already-deleted reference is just a CheckJNI warning.
+ // Currently, deleting an already-deleted reference is just a CheckJNI abort.
{
- bool old_check_jni = vm_->SetCheckJniEnabled(false);
- {
- CheckJniAbortCatcher check_jni_abort_catcher;
- env_->DeleteWeakGlobalRef(o);
- }
+ bool old_check_jni = vm_->SetCheckJniEnabled(true);
CheckJniAbortCatcher check_jni_abort_catcher;
- EXPECT_FALSE(vm_->SetCheckJniEnabled(true));
env_->DeleteWeakGlobalRef(o);
- std::string expected(StringPrintf("use of deleted weak global reference %p", o));
+ std::string expected(StringPrintf("jobject is an invalid weak global reference: %p", o));
check_jni_abort_catcher.Check(expected.c_str());
EXPECT_TRUE(vm_->SetCheckJniEnabled(old_check_jni));
}
diff --git a/runtime/thread.cc b/runtime/thread.cc
index 5584410..645c51f 100644
--- a/runtime/thread.cc
+++ b/runtime/thread.cc
@@ -2571,17 +2571,10 @@
// Local references do not need a read barrier.
result = locals.Get<kWithoutReadBarrier>(ref);
} else if (kind == kHandleScopeOrInvalid) {
- // TODO: make stack indirect reference table lookup more efficient.
- // Check if this is a local reference in the handle scope.
- if (LIKELY(HandleScopeContains(obj))) {
- // Read from handle scope.
- result = reinterpret_cast<StackReference<mirror::Object>*>(obj)->AsMirrorPtr();
- VerifyObject(result);
- } else {
- tlsPtr_.jni_env->vm_->JniAbortF(nullptr, "use of invalid jobject %p", obj);
- expect_null = true;
- result = nullptr;
- }
+ // Read from handle scope.
+ DCHECK(HandleScopeContains(obj));
+ result = reinterpret_cast<StackReference<mirror::Object>*>(obj)->AsMirrorPtr();
+ VerifyObject(result);
} else if (kind == kGlobal) {
result = tlsPtr_.jni_env->vm_->DecodeGlobal(ref);
} else {
@@ -2594,10 +2587,9 @@
}
}
- if (UNLIKELY(!expect_null && result == nullptr)) {
- tlsPtr_.jni_env->vm_->JniAbortF(nullptr, "use of deleted %s %p",
- ToStr<IndirectRefKind>(kind).c_str(), obj);
- }
+ DCHECK(expect_null || result != nullptr)
+ << "use of deleted " << ToStr<IndirectRefKind>(kind).c_str()
+ << " " << static_cast<const void*>(obj);
return result;
}