Do not cache RequiresConstructorBarrier() results.
Avoid caching the results. Caching was broken for JIT in the
presence of class unloading; entries for unloaded dex files
were leaked and potentially used erroneously with a newly
loaded dex file.
Test: m test-art-host-gtest
Test: testrunner.py --host
Test: Pixel 2 XL boots.
Test: m test-art-target-gtest
Test: testrunner.py --target
Bug: 118808764
Change-Id: Ic1163601170364e060c2e3009752f543c9bb37b7
diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc
index df6e8a8..8c276bb 100644
--- a/compiler/driver/compiler_driver.cc
+++ b/compiler/driver/compiler_driver.cc
@@ -254,7 +254,6 @@
verification_results_(verification_results),
compiler_(Compiler::Create(this, compiler_kind)),
compiler_kind_(compiler_kind),
- requires_constructor_barrier_lock_("constructor barrier lock"),
image_classes_(std::move(image_classes)),
number_of_soft_verifier_failures_(0),
had_hard_verifier_failure_(false),
@@ -1580,18 +1579,6 @@
self->ClearException();
}
-bool CompilerDriver::RequiresConstructorBarrier(const DexFile& dex_file,
- uint16_t class_def_idx) const {
- ClassAccessor accessor(dex_file, class_def_idx);
- // We require a constructor barrier if there are final instance fields.
- for (const ClassAccessor::Field& field : accessor.GetInstanceFields()) {
- if (field.IsFinal()) {
- return true;
- }
- }
- return false;
-}
-
class ResolveClassFieldsAndMethodsVisitor : public CompilationVisitor {
public:
explicit ResolveClassFieldsAndMethodsVisitor(const ParallelCompilationManager* manager)
@@ -1635,57 +1622,42 @@
// We want to resolve the methods and fields eagerly.
resolve_fields_and_methods = true;
}
- // If an instance field is final then we need to have a barrier on the return, static final
- // fields are assigned within the lock held for class initialization.
- bool requires_constructor_barrier = false;
- ClassAccessor accessor(dex_file, class_def_index);
- // Optionally resolve fields and methods and figure out if we need a constructor barrier.
- auto method_visitor = [&](const ClassAccessor::Method& method)
- REQUIRES_SHARED(Locks::mutator_lock_) {
- if (resolve_fields_and_methods) {
+ if (resolve_fields_and_methods) {
+ ClassAccessor accessor(dex_file, class_def_index);
+ // Optionally resolve fields and methods and figure out if we need a constructor barrier.
+ auto method_visitor = [&](const ClassAccessor::Method& method)
+ REQUIRES_SHARED(Locks::mutator_lock_) {
ArtMethod* resolved = class_linker->ResolveMethod<ClassLinker::ResolveMode::kNoChecks>(
method.GetIndex(),
dex_cache,
class_loader,
- /* referrer */ nullptr,
+ /*referrer=*/ nullptr,
method.GetInvokeType(class_def.access_flags_));
if (resolved == nullptr) {
CheckAndClearResolveException(soa.Self());
}
- }
- };
- accessor.VisitFieldsAndMethods(
- // static fields
- [&](ClassAccessor::Field& field) REQUIRES_SHARED(Locks::mutator_lock_) {
- if (resolve_fields_and_methods) {
+ };
+ accessor.VisitFieldsAndMethods(
+ // static fields
+ [&](ClassAccessor::Field& field) REQUIRES_SHARED(Locks::mutator_lock_) {
ArtField* resolved = class_linker->ResolveField(
- field.GetIndex(), dex_cache, class_loader, /* is_static */ true);
+ field.GetIndex(), dex_cache, class_loader, /*is_static=*/ true);
if (resolved == nullptr) {
CheckAndClearResolveException(soa.Self());
}
- }
- },
- // instance fields
- [&](ClassAccessor::Field& field) REQUIRES_SHARED(Locks::mutator_lock_) {
- if (field.IsFinal()) {
- // We require a constructor barrier if there are final instance fields.
- requires_constructor_barrier = true;
- }
- if (resolve_fields_and_methods) {
+ },
+ // instance fields
+ [&](ClassAccessor::Field& field) REQUIRES_SHARED(Locks::mutator_lock_) {
ArtField* resolved = class_linker->ResolveField(
- field.GetIndex(), dex_cache, class_loader, /* is_static */ false);
+ field.GetIndex(), dex_cache, class_loader, /*is_static=*/ false);
if (resolved == nullptr) {
CheckAndClearResolveException(soa.Self());
}
- }
- },
- /*direct methods*/ method_visitor,
- /*virtual methods*/ method_visitor);
- manager_->GetCompiler()->SetRequiresConstructorBarrier(self,
- &dex_file,
- class_def_index,
- requires_constructor_barrier);
+ },
+ /*direct_method_visitor=*/ method_visitor,
+ /*virtual_method_visitor=*/ method_visitor);
+ }
}
private:
@@ -2832,31 +2804,6 @@
return is_system_class;
}
-void CompilerDriver::SetRequiresConstructorBarrier(Thread* self,
- const DexFile* dex_file,
- uint16_t class_def_index,
- bool requires) {
- WriterMutexLock mu(self, requires_constructor_barrier_lock_);
- requires_constructor_barrier_.emplace(ClassReference(dex_file, class_def_index), requires);
-}
-
-bool CompilerDriver::RequiresConstructorBarrier(Thread* self,
- const DexFile* dex_file,
- uint16_t class_def_index) {
- ClassReference class_ref(dex_file, class_def_index);
- {
- ReaderMutexLock mu(self, requires_constructor_barrier_lock_);
- auto it = requires_constructor_barrier_.find(class_ref);
- if (it != requires_constructor_barrier_.end()) {
- return it->second;
- }
- }
- WriterMutexLock mu(self, requires_constructor_barrier_lock_);
- const bool requires = RequiresConstructorBarrier(*dex_file, class_def_index);
- requires_constructor_barrier_.emplace(class_ref, requires);
- return requires;
-}
-
std::string CompilerDriver::GetMemoryUsageString(bool extended) const {
std::ostringstream oss;
const gc::Heap* const heap = Runtime::Current()->GetHeap();
diff --git a/compiler/driver/compiler_driver.h b/compiler/driver/compiler_driver.h
index 9a83e55..f42e555 100644
--- a/compiler/driver/compiler_driver.h
+++ b/compiler/driver/compiler_driver.h
@@ -151,50 +151,6 @@
void AddCompiledMethod(const MethodReference& method_ref, CompiledMethod* const compiled_method);
CompiledMethod* RemoveCompiledMethod(const MethodReference& method_ref);
- void SetRequiresConstructorBarrier(Thread* self,
- const DexFile* dex_file,
- uint16_t class_def_index,
- bool requires)
- REQUIRES(!requires_constructor_barrier_lock_);
-
- // Do the <init> methods for this class require a constructor barrier (prior to the return)?
- // The answer is "yes", if and only if this class has any instance final fields.
- // (This must not be called for any non-<init> methods; the answer would be "no").
- //
- // ---
- //
- // JLS 17.5.1 "Semantics of final fields" mandates that all final fields are frozen at the end
- // of the invoked constructor. The constructor barrier is a conservative implementation means of
- // enforcing the freezes happen-before the object being constructed is observable by another
- // thread.
- //
- // Note: This question only makes sense for instance constructors;
- // static constructors (despite possibly having finals) never need
- // a barrier.
- //
- // JLS 12.4.2 "Detailed Initialization Procedure" approximately describes
- // class initialization as:
- //
- // lock(class.lock)
- // class.state = initializing
- // unlock(class.lock)
- //
- // invoke <clinit>
- //
- // lock(class.lock)
- // class.state = initialized
- // unlock(class.lock) <-- acts as a release
- //
- // The last operation in the above example acts as an atomic release
- // for any stores in <clinit>, which ends up being stricter
- // than what a constructor barrier needs.
- //
- // See also QuasiAtomic::ThreadFenceForConstructor().
- bool RequiresConstructorBarrier(Thread* self,
- const DexFile* dex_file,
- uint16_t class_def_index)
- REQUIRES(!requires_constructor_barrier_lock_);
-
// Resolve compiling method's class. Returns null on failure.
ObjPtr<mirror::Class> ResolveCompilingMethodsClass(const ScopedObjectAccess& soa,
Handle<mirror::DexCache> dex_cache,
@@ -407,20 +363,12 @@
void FreeThreadPools();
void CheckThreadPools();
- bool RequiresConstructorBarrier(const DexFile& dex_file, uint16_t class_def_idx) const;
-
const CompilerOptions* const compiler_options_;
VerificationResults* const verification_results_;
std::unique_ptr<Compiler> compiler_;
Compiler::Kind compiler_kind_;
- // All class references that require constructor barriers. If the class reference is not in the
- // set then the result has not yet been computed.
- mutable ReaderWriterMutex requires_constructor_barrier_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER;
- std::map<ClassReference, bool> requires_constructor_barrier_
- GUARDED_BY(requires_constructor_barrier_lock_);
-
// All class references that this compiler has compiled. Indexed by class defs.
using ClassStateTable = AtomicDexRefMap<ClassReference, ClassStatus>;
ClassStateTable compiled_classes_;
diff --git a/compiler/driver/dex_compilation_unit.cc b/compiler/driver/dex_compilation_unit.cc
index 654eccd..e5a6f0e 100644
--- a/compiler/driver/dex_compilation_unit.cc
+++ b/compiler/driver/dex_compilation_unit.cc
@@ -16,10 +16,14 @@
#include "dex_compilation_unit.h"
+#include "art_field.h"
#include "base/utils.h"
+#include "dex/class_accessor-inl.h"
#include "dex/code_item_accessors-inl.h"
#include "dex/descriptors_names.h"
+#include "mirror/class-inl.h"
#include "mirror/dex_cache.h"
+#include "scoped_thread_state_change-inl.h"
namespace art {
@@ -53,4 +57,32 @@
return symbol_;
}
+bool DexCompilationUnit::RequiresConstructorBarrier() const {
+ // Constructor barriers are applicable only for <init> methods.
+ DCHECK(!IsStatic());
+ DCHECK(IsConstructor());
+
+ // We require a constructor barrier if there are final instance fields.
+ if (GetCompilingClass().GetReference() != nullptr && !GetCompilingClass().IsNull()) {
+ // Decoding class data can be slow, so iterate over fields of the compiling class if resolved.
+ ScopedObjectAccess soa(Thread::Current());
+ ObjPtr<mirror::Class> compiling_class = GetCompilingClass().Get();
+ for (size_t i = 0, size = compiling_class->NumInstanceFields(); i != size; ++i) {
+ ArtField* field = compiling_class->GetInstanceField(i);
+ if (field->IsFinal()) {
+ return true;
+ }
+ }
+ } else {
+ // Iterate over field definitions in the class data.
+ ClassAccessor accessor(*GetDexFile(), GetClassDefIndex());
+ for (const ClassAccessor::Field& field : accessor.GetInstanceFields()) {
+ if (field.IsFinal()) {
+ return true;
+ }
+ }
+ }
+ return false;
+}
+
} // namespace art
diff --git a/compiler/driver/dex_compilation_unit.h b/compiler/driver/dex_compilation_unit.h
index 9d9c218..757f0e7 100644
--- a/compiler/driver/dex_compilation_unit.h
+++ b/compiler/driver/dex_compilation_unit.h
@@ -123,6 +123,41 @@
return compiling_class_;
}
+ // Does this <init> method require a constructor barrier (prior to the return)?
+ // The answer is "yes", if and only if the class has any instance final fields.
+ // (This must not be called for any non-<init> methods; the answer would be "no").
+ //
+ // ---
+ //
+ // JLS 17.5.1 "Semantics of final fields" mandates that all final fields are frozen at the end
+ // of the invoked constructor. The constructor barrier is a conservative implementation means of
+ // enforcing the freezes happen-before the object being constructed is observable by another
+ // thread.
+ //
+ // Note: This question only makes sense for instance constructors;
+ // static constructors (despite possibly having finals) never need
+ // a barrier.
+ //
+ // JLS 12.4.2 "Detailed Initialization Procedure" approximately describes
+ // class initialization as:
+ //
+ // lock(class.lock)
+ // class.state = initializing
+ // unlock(class.lock)
+ //
+ // invoke <clinit>
+ //
+ // lock(class.lock)
+ // class.state = initialized
+ // unlock(class.lock) <-- acts as a release
+ //
+ // The last operation in the above example acts as an atomic release
+ // for any stores in <clinit>, which ends up being stricter
+ // than what a constructor barrier needs.
+ //
+ // See also QuasiAtomic::ThreadFenceForConstructor().
+ bool RequiresConstructorBarrier() const;
+
private:
const Handle<mirror::ClassLoader> class_loader_;