[wasm] Share native modules compiled from the same bytes

Cache native modules in the wasm engine by their wire bytes. This is to
prepare for sharing {Script} objects between multiple {WasmModuleObject}
created from the same bytes. This also saves unnecessary compilation
time and memory.

R=clemensb@chromium.org

Bug: v8:6847
Change-Id: Iad5f70efbfe3f0f134dcb851edbcec50691677e0
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1916603
Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
Reviewed-by: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#65296}

src/wasm/module-compiler.cc

@@ -1356,6 +1356,15 @@ std::shared_ptr<NativeModule> CompileToNativeModule(
     std::shared_ptr<const WasmModule> module, const ModuleWireBytes& wire_bytes,
     Handle<FixedArray>* export_wrappers_out) {
   const WasmModule* wasm_module = module.get();
+  std::shared_ptr<NativeModule> native_module =
+      isolate->wasm_engine()->MaybeGetNativeModule(wasm_module->origin,
+                                                   wire_bytes.module_bytes());
+  if (native_module) {
+    // TODO(thibaudm): Look into sharing export wrappers.
+    CompileJsToWasmWrappers(isolate, wasm_module, export_wrappers_out);
+    return native_module;
+  }
+
   TimedHistogramScope wasm_compile_module_time_scope(SELECT_WASM_COUNTER(
       isolate->counters(), wasm_module->origin, wasm_compile, module_time));
 
@@ -1363,8 +1372,6 @@ std::shared_ptr<NativeModule> CompileToNativeModule(
   if (wasm_module->has_shared_memory) {
     isolate->CountUsage(v8::Isolate::UseCounterFeature::kWasmSharedMemory);
   }
-  // TODO(wasm): only save the sections necessary to deserialize a
-  // {WasmModule}. E.g. function bodies could be omitted.
   OwnedVector<uint8_t> wire_bytes_copy =
       OwnedVector<uint8_t>::Of(wire_bytes.module_bytes());
 
@@ -1373,11 +1380,13 @@ std::shared_ptr<NativeModule> CompileToNativeModule(
   size_t code_size_estimate =
       wasm::WasmCodeManager::EstimateNativeModuleCodeSize(module.get(),
                                                           uses_liftoff);
-  auto native_module = isolate->wasm_engine()->NewNativeModule(
+  native_module = isolate->wasm_engine()->NewNativeModule(
       isolate, enabled, std::move(module), code_size_estimate);
   native_module->SetWireBytes(std::move(wire_bytes_copy));
 
   CompileNativeModule(isolate, thrower, wasm_module, native_module.get());
+  isolate->wasm_engine()->UpdateNativeModuleCache(native_module,
+                                                  thrower->error());
   if (thrower->error()) return {};
 
   Impl(native_module->compilation_state())
@@ -2218,9 +2227,7 @@ bool AsyncStreamingProcessor::Deserialize(Vector<const uint8_t> module_bytes,
   job_->module_object_ =
       job_->isolate_->global_handles()->Create(*result.ToHandleChecked());
   job_->native_module_ = job_->module_object_->shared_native_module();
-  auto owned_wire_bytes = OwnedVector<uint8_t>::Of(wire_bytes);
-  job_->wire_bytes_ = ModuleWireBytes(owned_wire_bytes.as_vector());
-  job_->native_module_->SetWireBytes(std::move(owned_wire_bytes));
+  job_->wire_bytes_ = ModuleWireBytes(job_->native_module_->wire_bytes());
   job_->FinishCompile();
   return true;
 }

src/wasm/wasm-engine.cc

@@ -13,6 +13,7 @@
 #include "src/objects/heap-number.h"
 #include "src/objects/js-promise.h"
 #include "src/objects/objects-inl.h"
+#include "src/strings/string-hasher-inl.h"
 #include "src/utils/ostreams.h"
 #include "src/wasm/function-compiler.h"
 #include "src/wasm/module-compiler.h"
@@ -306,11 +307,6 @@ MaybeHandle<WasmModuleObject> WasmEngine::SyncCompile(
       CreateWasmScript(isolate, bytes, native_module->module()->source_map_url,
                        native_module->module()->name);
 
-  // Create the module object.
-  // TODO(clemensb): For the same module (same bytes / same hash), we should
-  // only have one WasmModuleObject. Otherwise, we might only set
-  // breakpoints on a (potentially empty) subset of the instances.
-
   // Create the compiled module object and populate with compiled functions
   // and information needed at instantiation time. This object needs to be
   // serializable. Instantiation may occur off a deserialized version of this
@@ -695,6 +691,42 @@ std::shared_ptr<NativeModule> WasmEngine::NewNativeModule(
   return native_module;
 }
 
+std::shared_ptr<NativeModule> WasmEngine::MaybeGetNativeModule(
+    ModuleOrigin origin, Vector<const uint8_t> wire_bytes) {
+  if (origin != kWasmOrigin) return nullptr;
+  base::MutexGuard lock(&mutex_);
+  while (true) {
+    auto it = native_module_cache_.find(wire_bytes);
+    if (it == native_module_cache_.end()) {
+      // Insert an empty entry to let other threads know that this
+      // {NativeModule} is already being created on another thread.
+      native_module_cache_.emplace(wire_bytes, std::weak_ptr<NativeModule>());
+      return nullptr;
+    }
+    if (auto shared_native_module = it->second.lock()) {
+      return shared_native_module;
+    }
+    cache_cv_.Wait(&mutex_);
+  }
+}
+
+void WasmEngine::UpdateNativeModuleCache(
+    std::shared_ptr<NativeModule> native_module, bool error) {
+  DCHECK_NOT_NULL(native_module);
+  if (native_module->module()->origin != kWasmOrigin) return;
+  Vector<const uint8_t> wire_bytes = native_module->wire_bytes();
+  base::MutexGuard lock(&mutex_);
+  auto it = native_module_cache_.find(wire_bytes);
+  DCHECK_NE(it, native_module_cache_.end());
+  DCHECK_NULL(it->second.lock());
+  // The lifetime of the temporary entry's bytes is unknown. Use the new native
+  // module's owned copy of the bytes for the key instead.
+  native_module_cache_.erase(it);
+  native_module_cache_.emplace(wire_bytes,
+                               error ? nullptr : std::move(native_module));
+  cache_cv_.NotifyAll();
+}
+
 void WasmEngine::FreeNativeModule(NativeModule* native_module) {
   base::MutexGuard guard(&mutex_);
   auto it = native_modules_.find(native_module);
@@ -735,6 +767,14 @@ void WasmEngine::FreeNativeModule(NativeModule* native_module) {
     TRACE_CODE_GC("Native module %p died, reducing dead code objects to %zu.\n",
                   native_module, current_gc_info_->dead_code.size());
   }
+  auto cache_it = native_module_cache_.find(native_module->wire_bytes());
+  // Not all native modules are stored in the cache currently. In particular
+  // asynchronous compilation and asmjs compilation results are not. So make
+  // sure that we only delete existing and expired entries.
+  if (cache_it != native_module_cache_.end() && cache_it->second.expired()) {
+    native_module_cache_.erase(cache_it);
+    cache_cv_.NotifyAll();
+  }
   native_modules_.erase(it);
 }
 
@@ -972,6 +1012,13 @@ std::shared_ptr<WasmEngine> WasmEngine::GetWasmEngine() {
   return *GetSharedWasmEngine();
 }
 
+size_t WasmEngine::WireBytesHasher::operator()(
+    const Vector<const uint8_t>& bytes) const {
+  return StringHasher::HashSequentialString(
+      reinterpret_cast<const char*>(bytes.begin()), bytes.length(),
+      kZeroHashSeed);
+}
+
 // {max_mem_pages} is declared in wasm-limits.h.
 uint32_t max_mem_pages() {
   STATIC_ASSERT(kV8MaxWasmMemoryPages <= kMaxUInt32);

src/wasm/wasm-engine.h

@@ -6,8 +6,11 @@
 #define V8_WASM_WASM_ENGINE_H_
 
 #include <memory>
+#include <unordered_map>
 #include <unordered_set>
 
+#include "src/base/platform/condition-variable.h"
+#include "src/base/platform/mutex.h"
 #include "src/tasks/cancelable-task.h"
 #include "src/wasm/wasm-code-manager.h"
 #include "src/wasm/wasm-tier.h"
@@ -182,6 +185,22 @@ class V8_EXPORT_PRIVATE WasmEngine {
       Isolate* isolate, const WasmFeatures& enabled_features,
       std::shared_ptr<const WasmModule> module, size_t code_size_estimate);
 
+  // Try getting a cached {NativeModule}. The {wire_bytes}' underlying array
+  // should be valid at least until the next call to {UpdateNativeModuleCache}.
+  // Return nullptr if no {NativeModule} exists for these bytes. In this case,
+  // an empty entry is added to let other threads know that a {NativeModule} for
+  // these bytes is currently being created. The caller should eventually call
+  // {UpdateNativeModuleCache} to update the entry and wake up other threads.
+  std::shared_ptr<NativeModule> MaybeGetNativeModule(
+      ModuleOrigin origin, Vector<const uint8_t> wire_bytes);
+
+  // Update the temporary cache entry inserted by {MaybeGetNativeModule}.
+  // Replace the key so that it uses the native module's owned copy of the
+  // bytes, and set the value to the new native module, or {nullptr} if {error}
+  // is true. Wake up the threads waiting for this {NativeModule}.
+  void UpdateNativeModuleCache(std::shared_ptr<NativeModule> native_module,
+                               bool error);
+
   void FreeNativeModule(NativeModule*);
 
   // Sample the code size of the given {NativeModule} in all isolates that have
@@ -275,6 +294,26 @@ class V8_EXPORT_PRIVATE WasmEngine {
   // about that.
   std::unique_ptr<CurrentGCInfo> current_gc_info_;
 
+  struct WireBytesHasher {
+    size_t operator()(const Vector<const uint8_t>& bytes) const;
+  };
+
+  // Native modules cached by their wire bytes.
+  // Each key points to the corresponding native module's wire bytes, so they
+  // should always be valid as long as the native module is alive.  When
+  // the native module dies, {FreeNativeModule} deletes the entry from the
+  // map, so that we do not leave any dangling key pointing to an expired
+  // weak_ptr. This also serves as a way to regularly clean up the map, which
+  // would otherwise accumulate expired entries.
+  std::unordered_map<Vector<const uint8_t>, std::weak_ptr<NativeModule>,
+                     WireBytesHasher>
+      native_module_cache_;
+
+  // This condition variable is used to synchronize threads compiling the same
+  // module. Only one thread will create the {NativeModule}. The other threads
+  // will wait on this variable until the first thread wakes them up.
+  base::ConditionVariable cache_cv_;
+
   // End of fields protected by {mutex_}.
   //////////////////////////////////////////////////////////////////////////////
 

src/wasm/wasm-serialization.cc

@@ -608,24 +608,40 @@ MaybeHandle<WasmModuleObject> DeserializeNativeModule(
 
   ModuleWireBytes wire_bytes(wire_bytes_vec);
   // TODO(titzer): module features should be part of the serialization format.
+  WasmEngine* wasm_engine = isolate->wasm_engine();
   WasmFeatures enabled_features = WasmFeatures::FromIsolate(isolate);
-  ModuleResult decode_result =
-      DecodeWasmModule(enabled_features, wire_bytes.start(), wire_bytes.end(),
-                       false, i::wasm::kWasmOrigin, isolate->counters(),
-                       isolate->wasm_engine()->allocator());
+  ModuleResult decode_result = DecodeWasmModule(
+      enabled_features, wire_bytes.start(), wire_bytes.end(), false,
+      i::wasm::kWasmOrigin, isolate->counters(), wasm_engine->allocator());
   if (decode_result.failed()) return {};
   std::shared_ptr<WasmModule> module = std::move(decode_result.value());
   CHECK_NOT_NULL(module);
   Handle<Script> script = CreateWasmScript(
       isolate, wire_bytes, module->source_map_url, module->name);
 
-  const bool kIncludeLiftoff = false;
-  size_t code_size_estimate =
-      wasm::WasmCodeManager::EstimateNativeModuleCodeSize(module.get(),
-                                                          kIncludeLiftoff);
-  auto shared_native_module = isolate->wasm_engine()->NewNativeModule(
-      isolate, enabled_features, std::move(module), code_size_estimate);
-  shared_native_module->SetWireBytes(OwnedVector<uint8_t>::Of(wire_bytes_vec));
+  auto shared_native_module =
+      wasm_engine->MaybeGetNativeModule(module->origin, wire_bytes_vec);
+  if (shared_native_module == nullptr) {
+    const bool kIncludeLiftoff = false;
+    size_t code_size_estimate =
+        wasm::WasmCodeManager::EstimateNativeModuleCodeSize(module.get(),
+                                                            kIncludeLiftoff);
+    shared_native_module = wasm_engine->NewNativeModule(
+        isolate, enabled_features, std::move(module), code_size_estimate);
+    shared_native_module->SetWireBytes(
+        OwnedVector<uint8_t>::Of(wire_bytes_vec));
+
+    NativeModuleDeserializer deserializer(shared_native_module.get());
+    WasmCodeRefScope wasm_code_ref_scope;
+
+    Reader reader(data + kVersionSize);
+    bool error = !deserializer.Read(&reader);
+    wasm_engine->UpdateNativeModuleCache(shared_native_module, error);
+    if (error) return {};
+  }
+
+  // Log the code within the generated module for profiling.
+  shared_native_module->LogWasmCodes(isolate);
 
   Handle<FixedArray> export_wrappers;
   CompileJsToWasmWrappers(isolate, shared_native_module->module(),
@@ -633,16 +649,6 @@ MaybeHandle<WasmModuleObject> DeserializeNativeModule(
 
   Handle<WasmModuleObject> module_object = WasmModuleObject::New(
       isolate, std::move(shared_native_module), script, export_wrappers);
-  NativeModule* native_module = module_object->native_module();
-
-  NativeModuleDeserializer deserializer(native_module);
-  WasmCodeRefScope wasm_code_ref_scope;
-
-  Reader reader(data + kVersionSize);
-  if (!deserializer.Read(&reader)) return {};
-
-  // Log the code within the generated module for profiling.
-  native_module->LogWasmCodes(isolate);
 
   // Finish the Wasm script now and make it public to the debugger.
   isolate->debug()->OnAfterCompile(script);

test/cctest/compiler/test-multiple-return.cc

@@ -125,8 +125,10 @@ std::shared_ptr<wasm::NativeModule> AllocateNativeModule(Isolate* isolate,
   // We have to add the code object to a NativeModule, because the
   // WasmCallDescriptor assumes that code is on the native heap and not
   // within a code object.
-  return isolate->wasm_engine()->NewNativeModule(
+  auto native_module = isolate->wasm_engine()->NewNativeModule(
       isolate, wasm::WasmFeatures::All(), std::move(module), code_size);
+  native_module->SetWireBytes({});
+  return native_module;
 }
 
 void TestReturnMultipleValues(MachineType type) {

test/cctest/wasm/test-wasm-import-wrapper-cache.cc

@@ -21,8 +21,10 @@ namespace test_wasm_import_wrapper_cache {
 std::shared_ptr<NativeModule> NewModule(Isolate* isolate) {
   std::shared_ptr<WasmModule> module(new WasmModule);
   constexpr size_t kCodeSizeEstimate = 16384;
-  return isolate->wasm_engine()->NewNativeModule(
+  auto native_module = isolate->wasm_engine()->NewNativeModule(
       isolate, WasmFeatures::All(), std::move(module), kCodeSizeEstimate);
+  native_module->SetWireBytes({});
+  return native_module;
 }
 
 TEST(CacheHit) {

test/fuzzer/multi-return.cc

@@ -142,8 +142,10 @@ std::shared_ptr<wasm::NativeModule> AllocateNativeModule(i::Isolate* isolate,
   // We have to add the code object to a NativeModule, because the
   // WasmCallDescriptor assumes that code is on the native heap and not
   // within a code object.
-  return isolate->wasm_engine()->NewNativeModule(
+  auto native_module = isolate->wasm_engine()->NewNativeModule(
       isolate, i::wasm::WasmFeatures::All(), std::move(module), code_size);
+  native_module->SetWireBytes({});
+  return native_module;
 }
 
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {

test/mjsunit/mjsunit.status

@@ -817,6 +817,10 @@
   # trigger a GC, but only in the isolate allocating the new memory.
   'wasm/module-memory': [SKIP],
   'wasm/shared-memory-gc-stress': [SKIP],
+
+  # Redirection to the interpreter is non-deterministic with multiple isolates.
+  'wasm/interpreter-mixed': [SKIP],
+  'wasm/worker-interpreter': [SKIP],
 }],  # 'isolates'
 
 ##############################################################################

test/mjsunit/wasm/interpreter-mixed.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --allow-natives-syntax
+// Flags: --allow-natives-syntax --expose-gc
 
 load('test/mjsunit/wasm/wasm-module-builder.js');
 
@@ -144,28 +144,32 @@ function redirectToInterpreter(
   // Three runs: Break in instance 1, break in instance 2, or both.
   for (let run = 0; run < 3; ++run) {
     print(" - run " + run);
-    let [instance1, instance2] = createTwoInstancesCallingEachOther();
-
-    let interpreted_before_1 = %WasmNumInterpretedCalls(instance1);
-    let interpreted_before_2 = %WasmNumInterpretedCalls(instance2);
-    // Call plus_two, which calls plus_one.
-    assertEquals(9, instance2.exports.plus_two(7));
-
-    // Nothing interpreted:
-    assertEquals(interpreted_before_1, %WasmNumInterpretedCalls(instance1));
-    assertEquals(interpreted_before_2, %WasmNumInterpretedCalls(instance2));
-
-    // Now redirect functions to the interpreter.
-    redirectToInterpreter(instance1, instance2, run != 1, run != 0);
-
-    // Call plus_two, which calls plus_one.
-    assertEquals(9, instance2.exports.plus_two(7));
-
-    // TODO(6668): Fix patching of instances which imported others' code.
-    //assertEquals(interpreted_before_1 + (run == 1 ? 0 : 1),
-    //             %WasmNumInterpretedCalls(instance1));
-    assertEquals(interpreted_before_2 + (run == 0 ? 0 : 1),
-                 %WasmNumInterpretedCalls(instance2));
+    (() => {
+      // Trigger a GC to ensure that the underlying native module is not a cached
+      // one from a previous run, with functions already redirected to the
+      // interpreter. This is not observable from pure JavaScript, but this is
+      // observable with the internal runtime functions used in this test.
+      // Run in a local scope to ensure previous native modules are
+      // unreachable.
+      gc();
+      let [instance1, instance2] = createTwoInstancesCallingEachOther();
+      let interpreted_before_1 = %WasmNumInterpretedCalls(instance1);
+      let interpreted_before_2 = %WasmNumInterpretedCalls(instance2);
+      // Call plus_two, which calls plus_one.
+      assertEquals(9, instance2.exports.plus_two(7));
+      // Nothing interpreted:
+      assertEquals(interpreted_before_1, %WasmNumInterpretedCalls(instance1));
+      assertEquals(interpreted_before_2, %WasmNumInterpretedCalls(instance2));
+      // Now redirect functions to the interpreter.
+      redirectToInterpreter(instance1, instance2, run != 1, run != 0);
+      // Call plus_two, which calls plus_one.
+      assertEquals(9, instance2.exports.plus_two(7));
+      // TODO(6668): Fix patching of instances which imported others' code.
+      //assertEquals(interpreted_before_1 + (run == 1 ? 0 : 1),
+      //             %WasmNumInterpretedCalls(instance1));
+      assertEquals(interpreted_before_2 + (run == 0 ? 0 : 1),
+                   %WasmNumInterpretedCalls(instance2))
+    })();
   }
 })();
 

test/mjsunit/wasm/worker-interpreter.js

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Flags: --allow-natives-syntax --no-wasm-disable-structured-cloning
+// Flags: --allow-natives-syntax --no-wasm-disable-structured-cloning --expose-gc
 
 load("test/mjsunit/wasm/wasm-module-builder.js");
 
@@ -12,6 +12,12 @@ load("test/mjsunit/wasm/wasm-module-builder.js");
     .addBody([kExprLocalGet, 0, kExprLocalGet, 1, kExprI32Add])
     .exportFunc();
 
+  // Trigger a GC to ensure that the underlying native module is not a cached
+  // one from a previous run, with functions already redirected to the
+  // interpreter. This is not observable from pure JavaScript, but this is
+  // observable with the internal runtime functions used in this test.
+  gc();
+
   let module = builder.toModule();
   let instance = new WebAssembly.Instance(module);
   let exp = instance.exports;