[wasm] Avoid redundant icache flushing

Code is often being patched after creating, thus we don't need to flush
the icache right away.
This CL introduces a new enum to specify whether the icache should be
flushed or not, and uses this in all methods which don't always need to
flush.

Drive-by: Fix a but where SKIP_ICACHE_FLUSH was interpreted as boolean
value.

R=mstarzinger@chromium.org

Change-Id: I13ac71d2a7168a065b8a4a1086c590816de8ca28
Reviewed-on: https://chromium-review.googlesource.com/971881Reviewed-by: Michael Starzinger <mstarzinger@chromium.org>
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#52102}

src/wasm/module-compiler.cc

@@ -913,7 +913,7 @@ wasm::WasmCode* EnsureExportedLazyDeoptData(Isolate* isolate,
     return code;
   }
   // Clone the lazy builtin into the native module.
-  return native_module->CloneLazyBuiltinInto(code, func_index);
+  return native_module->CloneLazyBuiltinInto(code, func_index, kFlushICache);
 }
 
 // Ensure that the code object in <code_table> at offset <func_index> has

src/wasm/wasm-code-manager.cc

@@ -56,7 +56,8 @@ const bool kModuleCanAllocateMoreMemory = true;
 
 void PatchTrampolineAndStubCalls(
     const WasmCode* original_code, const WasmCode* new_code,
-    const std::unordered_map<Address, Address, AddressHasher>& reverse_lookup) {
+    const std::unordered_map<Address, Address, AddressHasher>& reverse_lookup,
+    FlushICache flush_icache) {
   RelocIterator orig_it(
       original_code->instructions(), original_code->reloc_info(),
       original_code->constant_pool(), RelocInfo::kCodeTargetMask);
@@ -73,6 +74,10 @@ void PatchTrampolineAndStubCalls(
 #endif
     it.rinfo()->set_target_address(new_target, SKIP_WRITE_BARRIER);
   }
+  if (flush_icache) {
+    Assembler::FlushICache(new_code->instructions().start(),
+                           new_code->instructions().size());
+  }
 }
 
 }  // namespace
@@ -328,7 +333,7 @@ WasmCode* NativeModule::AddOwnedCode(
     uint32_t stack_slots, size_t safepoint_table_offset,
     size_t handler_table_offset,
     std::shared_ptr<ProtectedInstructions> protected_instructions,
-    WasmCode::Tier tier, bool flush_icache) {
+    WasmCode::Tier tier, FlushICache flush_icache) {
   // both allocation and insertion in owned_code_ happen in the same critical
   // section, thus ensuring owned_code_'s elements are rarely if ever moved.
   base::LockGuard<base::Mutex> lock(&allocation_mutex_);
@@ -402,16 +407,24 @@ WasmCode* NativeModule::AddAnonymousCode(Handle<Code> code,
   }
   std::shared_ptr<ProtectedInstructions> protected_instructions(
       new ProtectedInstructions(0));
-  WasmCode* ret = AddOwnedCode(
-      {code->instruction_start(),
-       static_cast<size_t>(code->instruction_size())},
-      std::move(reloc_info), static_cast<size_t>(code->relocation_size()),
-      Nothing<uint32_t>(), kind, code->constant_pool_offset(),
-      (code->has_safepoint_info() ? code->stack_slots() : 0),
-      (code->has_safepoint_info() ? code->safepoint_table_offset() : 0),
-      code->handler_table_offset(), protected_instructions, WasmCode::kOther,
-      false /* flush_icache */);
-  if (ret == nullptr) return nullptr;
+  Vector<const byte> orig_instructions(
+      code->instruction_start(), static_cast<size_t>(code->instruction_size()));
+  int stack_slots = code->has_safepoint_info() ? code->stack_slots() : 0;
+  int safepoint_table_offset =
+      code->has_safepoint_info() ? code->safepoint_table_offset() : 0;
+  WasmCode* ret =
+      AddOwnedCode(orig_instructions,      // instructions
+                   std::move(reloc_info),  // reloc_info
+                   static_cast<size_t>(code->relocation_size()),  // reloc_size
+                   Nothing<uint32_t>(),                           // index
+                   kind,                                          // kind
+                   code->constant_pool_offset(),  // constant_pool_offset
+                   stack_slots,                   // stack_slots
+                   safepoint_table_offset,        // safepoint_table_offset
+                   code->handler_table_offset(),  // handler_table_offset
+                   protected_instructions,        // protected_instructions
+                   WasmCode::kOther,              // kind
+                   kNoFlushICache);               // flush_icache
   intptr_t delta = ret->instructions().start() - code->instruction_start();
   int mask = RelocInfo::kApplyMask | RelocInfo::kCodeTargetMask |
              RelocInfo::ModeMask(RelocInfo::EMBEDDED_OBJECT);
@@ -457,8 +470,7 @@ WasmCode* NativeModule::AddCode(
       std::move(reloc_info), static_cast<size_t>(desc.reloc_size), Just(index),
       WasmCode::kFunction, desc.instr_size - desc.constant_pool_size,
       frame_slots, safepoint_table_offset, handler_table_offset,
-      std::move(protected_instructions), tier, SKIP_ICACHE_FLUSH);
-  if (ret == nullptr) return nullptr;
+      std::move(protected_instructions), tier, kNoFlushICache);
 
   code_table_[index] = ret;
   // TODO(mtrofin): this is a copy and paste from Code::CopyFrom.
@@ -507,10 +519,20 @@ Address NativeModule::CreateTrampolineTo(Handle<Code> code) {
   GenerateJumpTrampoline(&masm, dest);
   CodeDesc code_desc;
   masm.GetCode(nullptr, &code_desc);
-  WasmCode* wasm_code = AddOwnedCode(
-      {code_desc.buffer, static_cast<size_t>(code_desc.instr_size)}, nullptr, 0,
-      Nothing<uint32_t>(), WasmCode::kTrampoline, 0, 0, 0, 0, {},
-      WasmCode::kOther);
+  Vector<const byte> instructions(code_desc.buffer,
+                                  static_cast<size_t>(code_desc.instr_size));
+  WasmCode* wasm_code = AddOwnedCode(instructions,           // instructions
+                                     nullptr,                // reloc_info
+                                     0,                      // reloc_size
+                                     Nothing<uint32_t>(),    // index
+                                     WasmCode::kTrampoline,  // kind
+                                     0,   // constant_pool_offset
+                                     0,   // stack_slots
+                                     0,   // safepoint_table_offset
+                                     0,   // handler_table_offset
+                                     {},  // protected_instructions
+                                     WasmCode::kOther,  // tier
+                                     kFlushICache);     // flush_icache
   Address ret = wasm_code->instructions().start();
   trampolines_.emplace(std::make_pair(dest, ret));
   return ret;
@@ -644,15 +666,17 @@ WasmCode* NativeModule::Lookup(Address pc) {
 }
 
 WasmCode* NativeModule::CloneLazyBuiltinInto(const WasmCode* code,
-                                             uint32_t index) {
+                                             uint32_t index,
+                                             FlushICache flush_icache) {
   DCHECK_EQ(wasm::WasmCode::kLazyStub, code->kind());
-  WasmCode* ret = CloneCode(code);
+  WasmCode* ret = CloneCode(code, flush_icache);
   code_table_[index] = ret;
   ret->index_ = Just(index);
   return ret;
 }
 
-void NativeModule::CloneTrampolinesAndStubs(const NativeModule* other) {
+void NativeModule::CloneTrampolinesAndStubs(const NativeModule* other,
+                                            FlushICache flush_icache) {
   for (auto& pair : other->trampolines_) {
     Address key = pair.first;
     Address local =
@@ -662,13 +686,13 @@ void NativeModule::CloneTrampolinesAndStubs(const NativeModule* other) {
   }
   for (auto& pair : other->stubs_) {
     uint32_t key = pair.first;
-    WasmCode* clone = CloneCode(pair.second);
-    DCHECK_NOT_NULL(clone);
+    WasmCode* clone = CloneCode(pair.second, flush_icache);
     stubs_.emplace(std::make_pair(key, clone));
   }
 }
 
-WasmCode* NativeModule::CloneCode(const WasmCode* original_code) {
+WasmCode* NativeModule::CloneCode(const WasmCode* original_code,
+                                  FlushICache flush_icache) {
   std::unique_ptr<byte[]> reloc_info;
   if (original_code->reloc_info().size() > 0) {
     reloc_info.reset(new byte[original_code->reloc_info().size()]);
@@ -682,7 +706,7 @@ WasmCode* NativeModule::CloneCode(const WasmCode* original_code) {
       original_code->stack_slots(), original_code->safepoint_table_offset_,
       original_code->handler_table_offset_,
       original_code->protected_instructions_, original_code->tier(),
-      false /* flush_icache */);
+      kNoFlushICache);
   if (!ret->IsAnonymous()) {
     code_table_[ret->index()] = ret;
   }
@@ -693,10 +717,10 @@ WasmCode* NativeModule::CloneCode(const WasmCode* original_code) {
        !it.done(); it.next()) {
     it.rinfo()->apply(delta);
   }
-  // Flush the i-cache here instead of in AddOwnedCode, to include the changes
-  // made while iterating over the RelocInfo above.
-  Assembler::FlushICache(ret->instructions().start(),
-                         ret->instructions().size());
+  if (flush_icache) {
+    Assembler::FlushICache(ret->instructions().start(),
+                           ret->instructions().size());
+  }
   return ret;
 }
 
@@ -878,7 +902,9 @@ std::unique_ptr<NativeModule> NativeModule::Clone() {
   TRACE_HEAP("%zu cloned from %zu\n", ret->instance_id, instance_id);
   if (!ret) return ret;
 
-  ret->CloneTrampolinesAndStubs(this);
+  // Clone trampolines and stubs. They are later patched, so no icache flush
+  // needed yet.
+  ret->CloneTrampolinesAndStubs(this, kNoFlushICache);
 
   std::unordered_map<Address, Address, AddressHasher> reverse_lookup;
   for (auto& pair : trampolines_) {
@@ -900,7 +926,8 @@ std::unique_ptr<NativeModule> NativeModule::Clone() {
   for (auto& pair : ret->stubs_) {
     WasmCode* new_stub = pair.second;
     WasmCode* old_stub = stubs_.find(pair.first)->second;
-    PatchTrampolineAndStubCalls(old_stub, new_stub, reverse_lookup);
+    PatchTrampolineAndStubCalls(old_stub, new_stub, reverse_lookup,
+                                kFlushICache);
   }
 
   WasmCode* anonymous_lazy_builtin = nullptr;
@@ -913,23 +940,24 @@ std::unique_ptr<NativeModule> NativeModule::Clone() {
         // the {anonymous_lazy_builtin} variable. All non-anonymous such stubs
         // are just cloned directly via {CloneLazyBuiltinInto} below.
         if (!original_code->IsAnonymous()) {
-          WasmCode* new_code = ret->CloneLazyBuiltinInto(original_code, i);
-          if (new_code == nullptr) return nullptr;
-          PatchTrampolineAndStubCalls(original_code, new_code, reverse_lookup);
+          WasmCode* new_code =
+              ret->CloneLazyBuiltinInto(original_code, i, kNoFlushICache);
+          PatchTrampolineAndStubCalls(original_code, new_code, reverse_lookup,
+                                      kFlushICache);
           break;
         }
         if (anonymous_lazy_builtin == nullptr) {
-          WasmCode* new_code = ret->CloneCode(original_code);
-          if (new_code == nullptr) return nullptr;
-          PatchTrampolineAndStubCalls(original_code, new_code, reverse_lookup);
+          WasmCode* new_code = ret->CloneCode(original_code, kNoFlushICache);
+          PatchTrampolineAndStubCalls(original_code, new_code, reverse_lookup,
+                                      kFlushICache);
           anonymous_lazy_builtin = new_code;
         }
         ret->code_table_[i] = anonymous_lazy_builtin;
       } break;
       case WasmCode::kFunction: {
-        WasmCode* new_code = ret->CloneCode(original_code);
-        if (new_code == nullptr) return nullptr;
-        PatchTrampolineAndStubCalls(original_code, new_code, reverse_lookup);
+        WasmCode* new_code = ret->CloneCode(original_code, kNoFlushICache);
+        PatchTrampolineAndStubCalls(original_code, new_code, reverse_lookup,
+                                    kFlushICache);
       } break;
       default:
         UNREACHABLE();

src/wasm/wasm-code-manager.h

@@ -86,6 +86,8 @@ class V8_EXPORT_PRIVATE DisjointAllocationPool final {
 using ProtectedInstructions =
     std::vector<trap_handler::ProtectedInstructionData>;
 
+enum FlushICache : bool { kFlushICache = true, kNoFlushICache = false };
+
 class V8_EXPORT_PRIVATE WasmCode final {
  public:
   enum Kind {
@@ -239,7 +241,8 @@ class V8_EXPORT_PRIVATE NativeModule final {
   // builtin. The logic for seeking though frames would change, though.
   // TODO(mtrofin): perhaps we can do exactly that - either before or after
   // this change.
-  WasmCode* CloneLazyBuiltinInto(const WasmCode* code, uint32_t);
+  WasmCode* CloneLazyBuiltinInto(const WasmCode* code, uint32_t index,
+                                 FlushICache);
 
   bool SetExecutable(bool executable);
 
@@ -296,10 +299,10 @@ class V8_EXPORT_PRIVATE NativeModule final {
                          WasmCode::Kind kind, size_t constant_pool_offset,
                          uint32_t stack_slots, size_t safepoint_table_offset,
                          size_t handler_table_offset,
-                         std::shared_ptr<ProtectedInstructions>,
-                         WasmCode::Tier tier, bool flush_icache = true);
-  WasmCode* CloneCode(const WasmCode*);
-  void CloneTrampolinesAndStubs(const NativeModule* other);
+                         std::shared_ptr<ProtectedInstructions>, WasmCode::Tier,
+                         FlushICache);
+  WasmCode* CloneCode(const WasmCode*, FlushICache);
+  void CloneTrampolinesAndStubs(const NativeModule* other, FlushICache);
   WasmCode* Lookup(Address);
   Address GetLocalAddressFor(Handle<Code>);
   Address CreateTrampolineTo(Handle<Code>);
@@ -311,7 +314,11 @@ class V8_EXPORT_PRIVATE NativeModule final {
   std::vector<WasmCode*> code_table_;
   uint32_t num_imported_functions_;
 
+  // Maps from instruction start of an immovable code object to instruction
+  // start of the trampoline.
   std::unordered_map<Address, Address, AddressHasher> trampolines_;
+
+  // Maps from stub key to wasm code (containing a copy of that stub).
   std::unordered_map<uint32_t, WasmCode*> stubs_;
 
   std::unique_ptr<CompilationState, CompilationStateDeleter> compilation_state_;

src/wasm/wasm-serialization.cc

@@ -552,8 +552,7 @@ bool NativeModuleDeserializer::ReadCode() {
       code_buffer, std::move(reloc_info), reloc_size, Just(index_),
       WasmCode::kFunction, constant_pool_offset, stack_slot_count,
       safepoint_table_offset, handler_table_offset, protected_instructions,
-      tier, false /* flush_icache */);
-  if (ret == nullptr) return false;
+      tier, kNoFlushICache);
   native_module_->code_table_[index_] = ret;
 
   // now relocate the code