Revert "[wasm] Share native modules compiled from the same bytes"

MayaLekova · Commit Bot · commit 69fa5f794fb1 · 2019-12-02T16:51:44.000Z
This reverts commit c509bb8. Reason for revert: Breaks arm64 - sim - MSAN, see https://ci.chromium.org/p/v8/builders/ci/V8%20Linux%20-%20arm64%20-%20sim%20-%20MSAN/30050 Original change's description: > [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} TBR=clemensb@chromium.org,thibaudm@chromium.org Change-Id: I908b0f59bce26678d0b5d7fddc986384c40b4709 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: v8:6847 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1946334 Reviewed-by: Maya Lekova <mslekova@chromium.org> Commit-Queue: Maya Lekova <mslekova@chromium.org> Cr-Commit-Position: refs/heads/master@{#65297}
diff --git a/src/wasm/module-compiler.cc b/src/wasm/module-compiler.cc
@@ -1356,22 +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));
 
   // Embedder usage count for declared shared memories.
   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());
 
@@ -1380,13 +1373,11 @@ std::shared_ptr<NativeModule> CompileToNativeModule(
   size_t code_size_estimate =
       wasm::WasmCodeManager::EstimateNativeModuleCodeSize(module.get(),
                                                           uses_liftoff);
-  native_module = isolate->wasm_engine()->NewNativeModule(
+  auto 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())
@@ -2227,7 +2218,9 @@ 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();
-  job_->wire_bytes_ = ModuleWireBytes(job_->native_module_->wire_bytes());
+  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_->FinishCompile();
   return true;
 }
diff --git a/src/wasm/wasm-engine.cc b/src/wasm/wasm-engine.cc
@@ -13,7 +13,6 @@
 #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"
@@ -307,6 +306,11 @@ 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
@@ -691,42 +695,6 @@ 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);
@@ -767,14 +735,6 @@ 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);
 }
 
@@ -1012,13 +972,6 @@ 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);
diff --git a/src/wasm/wasm-engine.h b/src/wasm/wasm-engine.h
@@ -6,11 +6,8 @@
 #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"
@@ -185,22 +182,6 @@ 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
@@ -294,26 +275,6 @@ 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_}.
   //////////////////////////////////////////////////////////////////////////////
 
diff --git a/src/wasm/wasm-serialization.cc b/src/wasm/wasm-serialization.cc
@@ -608,47 +608,41 @@ 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(), wasm_engine->allocator());
+  ModuleResult decode_result =
+      DecodeWasmModule(enabled_features, wire_bytes.start(), wire_bytes.end(),
+                       false, i::wasm::kWasmOrigin, isolate->counters(),
+                       isolate->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);
 
-  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);
+  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));
 
   Handle<FixedArray> export_wrappers;
   CompileJsToWasmWrappers(isolate, shared_native_module->module(),
                           &export_wrappers);
 
   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);
diff --git a/test/cctest/compiler/test-multiple-return.cc b/test/cctest/compiler/test-multiple-return.cc
@@ -125,10 +125,8 @@ 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.
-  auto native_module = isolate->wasm_engine()->NewNativeModule(
+  return isolate->wasm_engine()->NewNativeModule(
       isolate, wasm::WasmFeatures::All(), std::move(module), code_size);
-  native_module->SetWireBytes({});
-  return native_module;
 }
 
 void TestReturnMultipleValues(MachineType type) {
diff --git a/test/cctest/wasm/test-wasm-import-wrapper-cache.cc b/test/cctest/wasm/test-wasm-import-wrapper-cache.cc
@@ -21,10 +21,8 @@ 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;
-  auto native_module = isolate->wasm_engine()->NewNativeModule(
+  return isolate->wasm_engine()->NewNativeModule(
       isolate, WasmFeatures::All(), std::move(module), kCodeSizeEstimate);
-  native_module->SetWireBytes({});
-  return native_module;
 }
 
 TEST(CacheHit) {
diff --git a/test/fuzzer/multi-return.cc b/test/fuzzer/multi-return.cc
@@ -142,10 +142,8 @@ 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.
-  auto native_module = isolate->wasm_engine()->NewNativeModule(
+  return 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) {
diff --git a/test/mjsunit/mjsunit.status b/test/mjsunit/mjsunit.status
@@ -817,10 +817,6 @@
   # 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'
 
 ##############################################################################
diff --git a/test/mjsunit/wasm/interpreter-mixed.js b/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 --expose-gc
+// Flags: --allow-natives-syntax
 
 load('test/mjsunit/wasm/wasm-module-builder.js');
 
@@ -144,32 +144,28 @@ function redirectToInterpreter(
   // Three runs: Break in instance 1, break in instance 2, or both.
   for (let run = 0; run < 3; ++run) {
     print(" - run " + run);
-    (() => {
-      // 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))
-    })();
+    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));
   }
 })();
 
diff --git a/test/mjsunit/wasm/worker-interpreter.js b/test/mjsunit/wasm/worker-interpreter.js
