From 371bd83f21f8db3b8e4cc8a660ead6a0650e92f6 Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Thu, 7 Apr 2016 10:19:48 -0700 Subject: Revert "Revert "Change RequiresConstructorBarrier default to yes"" This reverts commit 0436ee6bd33a0b905cd2a7e333f7935da1bd5d86. --- compiler/driver/compiler_driver.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'compiler/driver/compiler_driver.h') diff --git a/compiler/driver/compiler_driver.h b/compiler/driver/compiler_driver.h index 64a06a2f83..98e3d890cd 100644 --- a/compiler/driver/compiler_driver.h +++ b/compiler/driver/compiler_driver.h @@ -183,12 +183,12 @@ class CompilerDriver { // Remove and delete a compiled method. void RemoveCompiledMethod(const MethodReference& method_ref) REQUIRES(!compiled_methods_lock_); - void AddRequiresConstructorBarrier(Thread* self, const DexFile* dex_file, + void AddRequiresNoConstructorBarrier(Thread* self, const DexFile* dex_file, uint16_t class_def_index) - REQUIRES(!freezing_constructor_lock_); + REQUIRES(!no_barrier_constructor_classes_lock_); bool RequiresConstructorBarrier(Thread* self, const DexFile* dex_file, uint16_t class_def_index) const - REQUIRES(!freezing_constructor_lock_); + REQUIRES(!no_barrier_constructor_classes_lock_); // Callbacks from compiler to see what runtime checks must be generated. @@ -629,9 +629,10 @@ class CompilerDriver { const InstructionSet instruction_set_; const InstructionSetFeatures* const instruction_set_features_; - // All class references that require - mutable ReaderWriterMutex freezing_constructor_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; - std::set freezing_constructor_classes_ GUARDED_BY(freezing_constructor_lock_); + // All class references that do not require constructor barriers + mutable ReaderWriterMutex no_barrier_constructor_classes_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; + std::set no_barrier_constructor_classes_ + GUARDED_BY(no_barrier_constructor_classes_lock_); typedef SafeMap ClassTable; // All class references that this compiler has compiled. -- cgit v1.2.3-59-g8ed1b From b5d386118334fa5181c31b83b3fee6332537f4b7 Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Thu, 7 Apr 2016 10:52:52 -0700 Subject: Revert "Revert "Check if we require barrier if we did not resolve classes"" This reverts commit a7ab4997f7263439561093ffbc7dea29181a47c5. --- compiler/driver/compiler_driver.cc | 32 ++++++++++++++++++++++++++++++-- compiler/driver/compiler_driver.h | 8 +++++++- 2 files changed, 37 insertions(+), 3 deletions(-) (limited to 'compiler/driver/compiler_driver.h') diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc index 22e35ad634..5fe81c789c 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -358,6 +358,7 @@ CompilerDriver::CompilerDriver( instruction_set_(instruction_set), instruction_set_features_(instruction_set_features), no_barrier_constructor_classes_lock_("freezing constructor lock"), + resolved_classes_(false), compiled_classes_lock_("compiled classes lock"), compiled_methods_lock_("compiled method lock"), compiled_methods_(MethodTable::key_compare()), @@ -712,6 +713,8 @@ void CompilerDriver::Resolve(jobject class_loader, resolve_thread_count, timings); } + + resolved_classes_ = true; } // Resolve const-strings in the code. Done to have deterministic allocation behavior. Right now @@ -2006,6 +2009,28 @@ static void CheckAndClearResolveException(Thread* self) self->ClearException(); } +bool CompilerDriver::RequiresConstructorBarrier(const DexFile& dex_file, + uint16_t class_def_idx) const { + const DexFile::ClassDef& class_def = dex_file.GetClassDef(class_def_idx); + const uint8_t* class_data = dex_file.GetClassData(class_def); + if (class_data == nullptr) { + // Empty class such as a marker interface. + return false; + } + ClassDataItemIterator it(dex_file, class_data); + while (it.HasNextStaticField()) { + it.Next(); + } + // We require a constructor barrier if there are final instance fields. + while (it.HasNextInstanceField()) { + if (it.MemberIsFinal()) { + return true; + } + it.Next(); + } + return false; +} + class ResolveClassFieldsAndMethodsVisitor : public CompilationVisitor { public: explicit ResolveClassFieldsAndMethodsVisitor(const ParallelCompilationManager* manager) @@ -2779,8 +2804,11 @@ void CompilerDriver::AddRequiresNoConstructorBarrier(Thread* self, bool CompilerDriver::RequiresConstructorBarrier(Thread* self, const DexFile* dex_file, uint16_t class_def_index) const { - ReaderMutexLock mu(self, no_barrier_constructor_classes_lock_); - return no_barrier_constructor_classes_.count(ClassReference(dex_file, class_def_index)) == 0; + if (resolved_classes_) { + ReaderMutexLock mu(self, no_barrier_constructor_classes_lock_); + return no_barrier_constructor_classes_.count(ClassReference(dex_file, class_def_index)) == 0; + } + return RequiresConstructorBarrier(*dex_file, class_def_index); } std::string CompilerDriver::GetMemoryUsageString(bool extended) const { diff --git a/compiler/driver/compiler_driver.h b/compiler/driver/compiler_driver.h index 98e3d890cd..0ed0bb6af7 100644 --- a/compiler/driver/compiler_driver.h +++ b/compiler/driver/compiler_driver.h @@ -619,6 +619,8 @@ class CompilerDriver { 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_; DexFileToMethodInlinerMap* const method_inliner_map_; @@ -629,10 +631,14 @@ class CompilerDriver { const InstructionSet instruction_set_; const InstructionSetFeatures* const instruction_set_features_; - // All class references that do not require constructor barriers + // All class references that do not require constructor barriers. Only filled in if + // resolved_classes_ is true. mutable ReaderWriterMutex no_barrier_constructor_classes_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; std::set no_barrier_constructor_classes_ GUARDED_BY(no_barrier_constructor_classes_lock_); + // resolved_classes_ is true if we performed the resolve phase and filled in + // no_barrier_constructor_classes_. + bool resolved_classes_; typedef SafeMap ClassTable; // All class references that this compiler has compiled. -- cgit v1.2.3-59-g8ed1b From c4ae916def97b9e1ef6df35c8fabb3921a0e380c Mon Sep 17 00:00:00 2001 From: Mathieu Chartier Date: Thu, 7 Apr 2016 13:19:19 -0700 Subject: Store precice set of which constructors require barriers Fixes bugs where things in the boot image might not have been calculated even though resolved_clases was true. This only occured for app and test compiles though. Fixes test 476-checker-ctor-memory-barrier which was failing due to inlining something in the boot class path and getting a unexpected barrier since the barrier defaults to enabled. No measurable increase in RAM usage. Bug: 28005874 Change-Id: I4a417819aa129c95f4a83c38df1a66eb77824ea9 --- compiler/driver/compiler_driver.cc | 40 +++++++++++++++++------------- compiler/driver/compiler_driver.h | 28 ++++++++++----------- compiler/optimizing/instruction_builder.cc | 6 ++--- 3 files changed, 40 insertions(+), 34 deletions(-) (limited to 'compiler/driver/compiler_driver.h') diff --git a/compiler/driver/compiler_driver.cc b/compiler/driver/compiler_driver.cc index 5fe81c789c..52940687de 100644 --- a/compiler/driver/compiler_driver.cc +++ b/compiler/driver/compiler_driver.cc @@ -357,8 +357,7 @@ CompilerDriver::CompilerDriver( compiler_kind_(compiler_kind), instruction_set_(instruction_set), instruction_set_features_(instruction_set_features), - no_barrier_constructor_classes_lock_("freezing constructor lock"), - resolved_classes_(false), + requires_constructor_barrier_lock_("constructor barrier lock"), compiled_classes_lock_("compiled classes lock"), compiled_methods_lock_("compiled method lock"), compiled_methods_(MethodTable::key_compare()), @@ -713,8 +712,6 @@ void CompilerDriver::Resolve(jobject class_loader, resolve_thread_count, timings); } - - resolved_classes_ = true; } // Resolve const-strings in the code. Done to have deterministic allocation behavior. Right now @@ -2135,9 +2132,10 @@ class ResolveClassFieldsAndMethodsVisitor : public CompilationVisitor { DCHECK(!it.HasNext()); } } - if (!requires_constructor_barrier) { - manager_->GetCompiler()->AddRequiresNoConstructorBarrier(self, &dex_file, class_def_index); - } + manager_->GetCompiler()->SetRequiresConstructorBarrier(self, + &dex_file, + class_def_index, + requires_constructor_barrier); } private: @@ -2794,21 +2792,29 @@ size_t CompilerDriver::GetNonRelativeLinkerPatchCount() const { return non_relative_linker_patch_count_; } -void CompilerDriver::AddRequiresNoConstructorBarrier(Thread* self, - const DexFile* dex_file, - uint16_t class_def_index) { - WriterMutexLock mu(self, no_barrier_constructor_classes_lock_); - no_barrier_constructor_classes_.insert(ClassReference(dex_file, class_def_index)); +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) const { - if (resolved_classes_) { - ReaderMutexLock mu(self, no_barrier_constructor_classes_lock_); - return no_barrier_constructor_classes_.count(ClassReference(dex_file, class_def_index)) == 0; + 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; + } } - return RequiresConstructorBarrier(*dex_file, class_def_index); + 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 { diff --git a/compiler/driver/compiler_driver.h b/compiler/driver/compiler_driver.h index 0ed0bb6af7..905f84dd45 100644 --- a/compiler/driver/compiler_driver.h +++ b/compiler/driver/compiler_driver.h @@ -183,12 +183,15 @@ class CompilerDriver { // Remove and delete a compiled method. void RemoveCompiledMethod(const MethodReference& method_ref) REQUIRES(!compiled_methods_lock_); - void AddRequiresNoConstructorBarrier(Thread* self, const DexFile* dex_file, - uint16_t class_def_index) - REQUIRES(!no_barrier_constructor_classes_lock_); - bool RequiresConstructorBarrier(Thread* self, const DexFile* dex_file, - uint16_t class_def_index) const - REQUIRES(!no_barrier_constructor_classes_lock_); + void SetRequiresConstructorBarrier(Thread* self, + const DexFile* dex_file, + uint16_t class_def_index, + bool requires) + REQUIRES(!requires_constructor_barrier_lock_); + bool RequiresConstructorBarrier(Thread* self, + const DexFile* dex_file, + uint16_t class_def_index) + REQUIRES(!requires_constructor_barrier_lock_); // Callbacks from compiler to see what runtime checks must be generated. @@ -631,14 +634,11 @@ class CompilerDriver { const InstructionSet instruction_set_; const InstructionSetFeatures* const instruction_set_features_; - // All class references that do not require constructor barriers. Only filled in if - // resolved_classes_ is true. - mutable ReaderWriterMutex no_barrier_constructor_classes_lock_ DEFAULT_MUTEX_ACQUIRED_AFTER; - std::set no_barrier_constructor_classes_ - GUARDED_BY(no_barrier_constructor_classes_lock_); - // resolved_classes_ is true if we performed the resolve phase and filled in - // no_barrier_constructor_classes_. - bool resolved_classes_; + // 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 requires_constructor_barrier_ + GUARDED_BY(requires_constructor_barrier_lock_); typedef SafeMap ClassTable; // All class references that this compiler has compiled. diff --git a/compiler/optimizing/instruction_builder.cc b/compiler/optimizing/instruction_builder.cc index b0f0893720..06b39680b2 100644 --- a/compiler/optimizing/instruction_builder.cc +++ b/compiler/optimizing/instruction_builder.cc @@ -567,10 +567,10 @@ void HInstructionBuilder::Binop_22b(const Instruction& instruction, bool reverse UpdateLocal(instruction.VRegA(), current_block_->GetLastInstruction()); } -static bool RequiresConstructorBarrier(const DexCompilationUnit* cu, const CompilerDriver& driver) { +static bool RequiresConstructorBarrier(const DexCompilationUnit* cu, CompilerDriver* driver) { Thread* self = Thread::Current(); return cu->IsConstructor() - && driver.RequiresConstructorBarrier(self, cu->GetDexFile(), cu->GetClassDefIndex()); + && driver->RequiresConstructorBarrier(self, cu->GetDexFile(), cu->GetClassDefIndex()); } // Returns true if `block` has only one successor which starts at the next @@ -616,7 +616,7 @@ void HInstructionBuilder::BuildReturn(const Instruction& instruction, if (graph_->ShouldGenerateConstructorBarrier()) { // The compilation unit is null during testing. if (dex_compilation_unit_ != nullptr) { - DCHECK(RequiresConstructorBarrier(dex_compilation_unit_, *compiler_driver_)) + DCHECK(RequiresConstructorBarrier(dex_compilation_unit_, compiler_driver_)) << "Inconsistent use of ShouldGenerateConstructorBarrier. Should not generate a barrier."; } AppendInstruction(new (arena_) HMemoryBarrier(kStoreStore, dex_pc)); -- cgit v1.2.3-59-g8ed1b