Faster `ArtMethod` fixup in `ImageWriter`.

Calculate the access flags in a local variable to avoid
unnecessary atomic operations. Clean up the code to avoid
some unnecessary branching. Avoid nterp flag recalculation
when the target ISA is the same as `kRuntimeISA`.

Test: m test-art-host-gtest
Test: testrunner.py --host --optimizing
Bug: 181943478
Change-Id: I2486d54f4b4cf73f9b8e94b366ee9db3085e5fa7

4 files changed, 84 insertions, 64 deletions

diff --git a/dex2oat/linker/image_writer.cc b/dex2oat/linker/image_writer.cc
index abc3354421..9c83da5491 100644
--- a/dex2oat/linker/image_writer.cc
+++ b/dex2oat/linker/image_writer.cc
@@ -452,8 +452,6 @@ static void ClearDexFileCookies() REQUIRES_SHARED(Locks::mutator_lock_) {
 }
 
 bool ImageWriter::PrepareImageAddressSpace(TimingLogger* timings) {
-  target_ptr_size_ = InstructionSetPointerSize(compiler_options_.GetInstructionSet());
-
   Thread* const self = Thread::Current();
 
   gc::Heap* const heap = Runtime::Current()->GetHeap();
@@ -3506,35 +3504,52 @@ const uint8_t* ImageWriter::GetQuickCode(ArtMethod* method, const ImageInfo& ima
   return quick_code;
 }
 
+static inline uint32_t ResetNterpFastPathFlags(
+    uint32_t access_flags, ArtMethod* orig, InstructionSet isa)
+    REQUIRES_SHARED(Locks::mutator_lock_) {
+  DCHECK(orig != nullptr);
+  DCHECK(!orig->IsProxyMethod());  // `UnstartedRuntime` does not support creating proxy classes.
+  DCHECK(!orig->IsRuntimeMethod());
+
+  // Clear old nterp fast path flags.
+  access_flags = ArtMethod::ClearNterpFastPathFlags(access_flags);
+
+  // Check if nterp fast paths are available on the target ISA.
+  std::string_view shorty = orig->GetShortyView();  // Use orig, copy's class not yet ready.
+  uint32_t new_nterp_flags = GetNterpFastPathFlags(shorty, access_flags, isa);
+
+  // Add the new nterp fast path flags, if any.
+  return access_flags | new_nterp_flags;
+}
+
 void ImageWriter::CopyAndFixupMethod(ArtMethod* orig,
                                      ArtMethod* copy,
                                      size_t oat_index) {
-  if (orig->IsAbstract()) {
-    // Ignore the single-implementation info for abstract method.
-    // Do this on orig instead of copy, otherwise there is a crash due to methods
-    // are copied before classes.
-    // TODO: handle fixup of single-implementation method for abstract method.
-    orig->SetHasSingleImplementation(false);
-    orig->SetSingleImplementation(
-        nullptr, Runtime::Current()->GetClassLinker()->GetImagePointerSize());
-  }
-
-  if (!orig->IsRuntimeMethod()) {
-    // If we're compiling a boot image and we have a profile, set methods as
-    // being shared memory (to avoid dirtying them with hotness counter). We
-    // expect important methods to be AOT, and non-important methods to be run
-    // in the interpreter.
-    if (CompilerFilter::DependsOnProfile(compiler_options_.GetCompilerFilter()) &&
-        (compiler_options_.IsBootImage() || compiler_options_.IsBootImageExtension())) {
-      orig->SetMemorySharedMethod();
-    }
-  }
-
   memcpy(copy, orig, ArtMethod::Size(target_ptr_size_));
 
   CopyAndFixupReference(copy->GetDeclaringClassAddressWithoutBarrier(),
                         orig->GetDeclaringClassUnchecked<kWithoutReadBarrier>());
-  ResetNterpFastPathFlags(copy, orig);
+
+  if (!orig->IsRuntimeMethod()) {
+    uint32_t access_flags = orig->GetAccessFlags();
+    if (ArtMethod::IsAbstract(access_flags)) {
+      // Ignore the single-implementation info for abstract method.
+      // TODO: handle fixup of single-implementation method for abstract method.
+      access_flags = ArtMethod::SetHasSingleImplementation(access_flags, /*single_impl=*/ false);
+      copy->SetSingleImplementation(nullptr, target_ptr_size_);
+    } else if (mark_memory_shared_methods_ && LIKELY(!ArtMethod::IsIntrinsic(access_flags))) {
+      access_flags = ArtMethod::SetMemorySharedMethod(access_flags);
+      copy->SetHotCounter();
+    }
+
+    InstructionSet isa = compiler_options_.GetInstructionSet();
+    if (isa != kRuntimeISA) {
+      access_flags = ResetNterpFastPathFlags(access_flags, orig, isa);
+    } else {
+      DCHECK_EQ(access_flags, ResetNterpFastPathFlags(access_flags, orig, isa));
+    }
+    copy->SetAccessFlags(access_flags);
+  }
 
   // OatWriter replaces the code_ with an offset value. Here we re-adjust to a pointer relative to
   // oat_begin_
@@ -3754,11 +3769,17 @@ ImageWriter::ImageWriter(const CompilerOptions& compiler_options,
                          jobject class_loader,
                          const std::vector<std::string>* dirty_image_objects)
     : compiler_options_(compiler_options),
+      target_ptr_size_(InstructionSetPointerSize(compiler_options.GetInstructionSet())),
+      // If we're compiling a boot image and we have a profile, set methods as being shared
+      // memory (to avoid dirtying them with hotness counter). We expect important methods
+      // to be AOT, and non-important methods to be run in the interpreter.
+      mark_memory_shared_methods_(
+          CompilerFilter::DependsOnProfile(compiler_options_.GetCompilerFilter()) &&
+              (compiler_options_.IsBootImage() || compiler_options_.IsBootImageExtension())),
       boot_image_begin_(Runtime::Current()->GetHeap()->GetBootImagesStartAddress()),
       boot_image_size_(Runtime::Current()->GetHeap()->GetBootImagesSize()),
       global_image_begin_(reinterpret_cast<uint8_t*>(image_begin)),
       image_objects_offset_begin_(0),
-      target_ptr_size_(InstructionSetPointerSize(compiler_options.GetInstructionSet())),
       image_infos_(oat_filenames.size()),
       jni_stub_map_(JniStubKeyHash(compiler_options.GetInstructionSet()),
                     JniStubKeyEquals(compiler_options.GetInstructionSet())),
@@ -3841,32 +3862,5 @@ void ImageWriter::CopyAndFixupPointer(void* object, MemberOffset offset, ValueTy
   return CopyAndFixupPointer(object, offset, src_value, target_ptr_size_);
 }
 
-void ImageWriter::ResetNterpFastPathFlags(ArtMethod* copy, ArtMethod* orig) {
-  DCHECK(copy != nullptr);
-  DCHECK(orig != nullptr);
-  if (orig->IsRuntimeMethod() || orig->IsProxyMethod()) {
-    return;  // !IsRuntimeMethod() and !IsProxyMethod() for GetShortyView()
-  }
-
-  // Clear old nterp fast path flags.
-  if (copy->HasNterpEntryPointFastPathFlag()) {
-    copy->ClearNterpEntryPointFastPathFlag();  // Flag has other uses, clear it conditionally.
-  }
-  copy->ClearNterpInvokeFastPathFlag();
-
-  // Check if nterp fast paths available on target ISA.
-  std::string_view shorty = orig->GetShortyView();  // Use orig, copy's class not yet ready.
-  uint32_t new_nterp_flags =
-      GetNterpFastPathFlags(shorty, copy->GetAccessFlags(), compiler_options_.GetInstructionSet());
-
-  // Set new nterp fast path flags, if approporiate.
-  if ((new_nterp_flags & kAccNterpEntryPointFastPathFlag) != 0) {
-    copy->SetNterpEntryPointFastPathFlag();
-  }
-  if ((new_nterp_flags & kAccNterpInvokeFastPathFlag) != 0) {
-    copy->SetNterpInvokeFastPathFlag();
-  }
-}
-
 }  // namespace linker
 }  // namespace art
diff --git a/dex2oat/linker/image_writer.h b/dex2oat/linker/image_writer.h
index 8ffe226bec..efe8fa2304 100644
--- a/dex2oat/linker/image_writer.h
+++ b/dex2oat/linker/image_writer.h
@@ -618,9 +618,6 @@ class ImageWriter final {
   void CopyAndFixupPointer(void* object, MemberOffset offset, ValueType src_value)
       REQUIRES_SHARED(Locks::mutator_lock_);
 
-  void ResetNterpFastPathFlags(ArtMethod* copy, ArtMethod* orig)
-      REQUIRES_SHARED(Locks::mutator_lock_);
-
   ALWAYS_INLINE
   static bool IsStronglyInternedString(ObjPtr<mirror::String> str)
       REQUIRES_SHARED(Locks::mutator_lock_);
@@ -639,6 +636,12 @@ class ImageWriter final {
 
   const CompilerOptions& compiler_options_;
 
+  // Size of pointers on the target architecture.
+  PointerSize target_ptr_size_;
+
+  // Whether to mark non-abstract, non-intrinsic methods as "memory shared methods".
+  bool mark_memory_shared_methods_;
+
   // Cached boot image begin and size. This includes heap, native objects and oat files.
   const uint32_t boot_image_begin_;
   const uint32_t boot_image_size_;
@@ -656,9 +659,6 @@ class ImageWriter final {
   // Oat index map for objects.
   HashMap<mirror::Object*, uint32_t> oat_index_map_;
 
-  // Size of pointers on the target architecture.
-  PointerSize target_ptr_size_;
-
   // Image data indexed by the oat file index.
   dchecked_vector<ImageInfo> image_infos_;
 
diff --git a/runtime/art_method.h b/runtime/art_method.h
index bf1b4631c6..a23969aa73 100644
--- a/runtime/art_method.h
+++ b/runtime/art_method.h
@@ -359,11 +359,15 @@ class EXPORT ArtMethod final {
   }
 
   void SetMemorySharedMethod() REQUIRES_SHARED(Locks::mutator_lock_) {
-    uint32_t access_flags = GetAccessFlags();
-    if (!IsIntrinsic(access_flags) && !IsAbstract(access_flags)) {
-      AddAccessFlags(kAccMemorySharedMethod);
-      SetHotCounter();
-    }
+    DCHECK(!IsIntrinsic());
+    DCHECK(!IsAbstract());
+    AddAccessFlags(kAccMemorySharedMethod);
+  }
+
+  static uint32_t SetMemorySharedMethod(uint32_t access_flags) {
+    DCHECK(!IsIntrinsic(access_flags));
+    DCHECK(!IsAbstract(access_flags));
+    return access_flags | kAccMemorySharedMethod;
   }
 
   void ClearMemorySharedMethod() REQUIRES_SHARED(Locks::mutator_lock_) {
@@ -597,6 +601,15 @@ class EXPORT ArtMethod final {
     ClearAccessFlags(kAccNterpInvokeFastPathFlag);
   }
 
+  static uint32_t ClearNterpFastPathFlags(uint32_t access_flags) {
+    // `kAccNterpEntryPointFastPathFlag` has a different use for native methods.
+    if (!IsNative(access_flags)) {
+      access_flags &= ~kAccNterpEntryPointFastPathFlag;
+    }
+    access_flags &= ~kAccNterpInvokeFastPathFlag;
+    return access_flags;
+  }
+
   // Returns whether the method is a string constructor. The method must not
   // be a class initializer. (Class initializers are called from a different
   // context where we do not need to check for string constructors.)
@@ -810,6 +823,15 @@ class EXPORT ArtMethod final {
     return (GetAccessFlags() & kAccSingleImplementation) != 0;
   }
 
+  static uint32_t SetHasSingleImplementation(uint32_t access_flags, bool single_impl) {
+    DCHECK(!IsIntrinsic(access_flags)) << "conflict with intrinsic bits";
+    if (single_impl) {
+      return access_flags | kAccSingleImplementation;
+    } else {
+      return access_flags & ~kAccSingleImplementation;
+    }
+  }
+
   // Takes a method and returns a 'canonical' one if the method is default (and therefore
   // potentially copied from some other class). For example, this ensures that the debugger does not
   // get confused as to which method we are in.
diff --git a/runtime/class_linker.cc b/runtime/class_linker.cc
index 7359056d32..52caa9f1cf 100644
--- a/runtime/class_linker.cc
+++ b/runtime/class_linker.cc
@@ -4163,9 +4163,13 @@ void ClassLinker::LoadMethod(const DexFile& dex_file,
     }
   }
 
-  if (Runtime::Current()->IsZygote() &&
+  if ((access_flags & kAccAbstract) == 0u &&
+      Runtime::Current()->IsZygote() &&
       !Runtime::Current()->GetJITOptions()->GetProfileSaverOptions().GetProfileBootClassPath()) {
+    DCHECK(!ArtMethod::IsAbstract(access_flags));
+    DCHECK(!ArtMethod::IsIntrinsic(access_flags));
     dst->SetMemorySharedMethod();
+    dst->SetHotCounter();
   }
 }