Hold intern table lock for AddImageStringsToTable
Fixes a correctness issue where another thread adding an intern string
after the visitor could cause duplicate strings.
Reduces how often the intern table lock is acquired, probably
improving performance.
Bug: 116059983
Test: test-art-host
Change-Id: I5ba6ca3ba7535de6d4ad5cb46750bd23a6e9aadc
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index f43791a..e3dfdb3 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -1486,7 +1486,6 @@
// the strings they point to.
ScopedTrace timing("AppImage:InternString");
- Thread* const self = Thread::Current();
InternTable* const intern_table = Runtime::Current()->GetInternTable();
// Add the intern table, removing any conflicts. For conflicts, store the new address in a map
@@ -1494,13 +1493,14 @@
// TODO: Optimize with a bitmap or bloom filter
SafeMap<mirror::String*, mirror::String*> intern_remap;
intern_table->AddImageStringsToTable(space, [&](InternTable::UnorderedSet& interns)
- REQUIRES_SHARED(Locks::mutator_lock_) {
+ REQUIRES_SHARED(Locks::mutator_lock_)
+ REQUIRES(Locks::intern_table_lock_) {
VLOG(image) << "AppImage:stringsInInternTableSize = " << interns.size();
for (auto it = interns.begin(); it != interns.end(); ) {
ObjPtr<mirror::String> string = it->Read();
- ObjPtr<mirror::String> existing = intern_table->LookupWeak(self, string);
+ ObjPtr<mirror::String> existing = intern_table->LookupWeakLocked(string);
if (existing == nullptr) {
- existing = intern_table->LookupStrong(self, string);
+ existing = intern_table->LookupStrongLocked(string);
}
if (existing != nullptr) {
intern_remap.Put(string.Ptr(), existing.Ptr());
diff --git a/runtime/intern_table-inl.h b/runtime/intern_table-inl.h
index 8c7fb42..84754fa 100644
--- a/runtime/intern_table-inl.h
+++ b/runtime/intern_table-inl.h
@@ -39,11 +39,15 @@
inline size_t InternTable::AddTableFromMemory(const uint8_t* ptr, const Visitor& visitor) {
size_t read_count = 0;
UnorderedSet set(ptr, /*make copy*/false, &read_count);
- // Visit the unordered set, may remove elements.
- visitor(set);
- if (!set.empty()) {
+ {
+ // Hold the lock while calling the visitor to prevent possible race
+ // conditions with another thread adding intern strings.
MutexLock mu(Thread::Current(), *Locks::intern_table_lock_);
- strong_interns_.AddInternStrings(std::move(set));
+ // Visit the unordered set, may remove elements.
+ visitor(set);
+ if (!set.empty()) {
+ strong_interns_.AddInternStrings(std::move(set));
+ }
}
return read_count;
}
diff --git a/runtime/intern_table.h b/runtime/intern_table.h
index 1bc89a1..e918a45 100644
--- a/runtime/intern_table.h
+++ b/runtime/intern_table.h
@@ -145,11 +145,15 @@
ObjPtr<mirror::String> LookupStrong(Thread* self, uint32_t utf16_length, const char* utf8_data)
REQUIRES(!Locks::intern_table_lock_)
REQUIRES_SHARED(Locks::mutator_lock_);
+ ObjPtr<mirror::String> LookupStrongLocked(ObjPtr<mirror::String> s)
+ REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(Locks::intern_table_lock_);
// Lookup a weak intern, returns null if not found.
ObjPtr<mirror::String> LookupWeak(Thread* self, ObjPtr<mirror::String> s)
REQUIRES(!Locks::intern_table_lock_)
REQUIRES_SHARED(Locks::mutator_lock_);
+ ObjPtr<mirror::String> LookupWeakLocked(ObjPtr<mirror::String> s)
+ REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(Locks::intern_table_lock_);
// Total number of interned strings.
size_t Size() const REQUIRES(!Locks::intern_table_lock_);
@@ -250,10 +254,6 @@
size_t AddTableFromMemory(const uint8_t* ptr, const Visitor& visitor)
REQUIRES(!Locks::intern_table_lock_) REQUIRES_SHARED(Locks::mutator_lock_);
- ObjPtr<mirror::String> LookupStrongLocked(ObjPtr<mirror::String> s)
- REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(Locks::intern_table_lock_);
- ObjPtr<mirror::String> LookupWeakLocked(ObjPtr<mirror::String> s)
- REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(Locks::intern_table_lock_);
ObjPtr<mirror::String> InsertStrong(ObjPtr<mirror::String> s)
REQUIRES_SHARED(Locks::mutator_lock_) REQUIRES(Locks::intern_table_lock_);
ObjPtr<mirror::String> InsertWeak(ObjPtr<mirror::String> s)
