Reland "[wasm] Cleanup wasm script creation"

This is a reland of f902ef32

Original change's description:
> [wasm] Cleanup wasm script creation
> 
> - Do not expose CreateWasmScript since we should now use
> WasmEngine:GetOrCreateScript instead,
> - Initialize all Script fields in CreateWasmScript, not in
> WasmModuleObject::New,
> - Do not pass code size estimate argument, since we can always use the
> actual native module's committed code space.
> 
> R=clemensb@chromium.org
> 
> Bug: v8:10349
> Change-Id: If9250d62ffc271ab6efc3b9c45958a305c9d1827
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2135633
> Reviewed-by: Clemens Backes <clemensb@chromium.org>
> Commit-Queue: Thibaud Michaud <thibaudm@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#67083}

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

src/objects/managed.h

@@ -92,11 +92,12 @@ class Managed : public Foreign {
   // Create a {Managed<CppType>} from an existing {std::shared_ptr<CppType>}.
   static Handle<Managed<CppType>> FromSharedPtr(
       Isolate* isolate, size_t estimated_size,
-      const std::shared_ptr<CppType>& shared_ptr) {
+      std::shared_ptr<CppType> shared_ptr) {
     reinterpret_cast<v8::Isolate*>(isolate)
         ->AdjustAmountOfExternalAllocatedMemory(estimated_size);
     auto destructor = new ManagedPtrDestructor(
-        estimated_size, new std::shared_ptr<CppType>{shared_ptr}, Destructor);
+        estimated_size, new std::shared_ptr<CppType>{std::move(shared_ptr)},
+        Destructor);
     Handle<Managed<CppType>> handle = Handle<Managed<CppType>>::cast(
         isolate->factory()->NewForeign(reinterpret_cast<Address>(destructor)));
     Handle<Object> global_handle = isolate->global_handles()->Create(*handle);

src/wasm/module-compiler.cc

@@ -1602,11 +1602,9 @@ void AsyncCompileJob::PrepareRuntimeObjects() {
   // Create heap objects for script and module bytes to be stored in the
   // module object. Asm.js is not compiled asynchronously.
   DCHECK(module_object_.is_null());
-  const WasmModule* module = native_module_->module();
   auto source_url = stream_ ? stream_->url() : Vector<const char>();
   auto script = isolate_->wasm_engine()->GetOrCreateScript(
-      isolate_, native_module_.get(), VectorOf(module->source_map_url),
-      module->name, source_url);
+      isolate_, native_module_, source_url);
   Handle<WasmModuleObject> module_object =
       WasmModuleObject::New(isolate_, native_module_, script);
 
@@ -2996,71 +2994,6 @@ WasmCode* CompileImportWrapper(
   return published_code;
 }
 
-Handle<Script> CreateWasmScript(Isolate* isolate,
-                                Vector<const uint8_t> wire_bytes,
-                                Vector<const char> source_map_url,
-                                WireBytesRef name,
-                                Vector<const char> source_url) {
-  Handle<Script> script =
-      isolate->factory()->NewScript(isolate->factory()->empty_string());
-  script->set_compilation_state(Script::COMPILATION_STATE_COMPILED);
-  script->set_context_data(isolate->native_context()->debug_context_id());
-  script->set_type(Script::TYPE_WASM);
-
-  int hash = StringHasher::HashSequentialString(
-      reinterpret_cast<const char*>(wire_bytes.begin()), wire_bytes.length(),
-      kZeroHashSeed);
-
-  const int kBufferSize = 32;
-  char buffer[kBufferSize];
-
-  // Script name is "<module_name>-hash" if name is available and "hash"
-  // otherwise.
-  Handle<String> name_str;
-  if (name.is_set()) {
-    int name_chars = SNPrintF(ArrayVector(buffer), "-%08x", hash);
-    DCHECK(name_chars >= 0 && name_chars < kBufferSize);
-    Handle<String> name_hash =
-        isolate->factory()
-            ->NewStringFromOneByte(
-                VectorOf(reinterpret_cast<uint8_t*>(buffer), name_chars),
-                AllocationType::kOld)
-            .ToHandleChecked();
-    Handle<String> module_name =
-        WasmModuleObject::ExtractUtf8StringFromModuleBytes(
-            isolate, wire_bytes, name, kNoInternalize);
-    name_str = isolate->factory()
-                   ->NewConsString(module_name, name_hash)
-                   .ToHandleChecked();
-  } else {
-    int name_chars = SNPrintF(ArrayVector(buffer), "%08x", hash);
-    DCHECK(name_chars >= 0 && name_chars < kBufferSize);
-    name_str = isolate->factory()
-                   ->NewStringFromOneByte(
-                       VectorOf(reinterpret_cast<uint8_t*>(buffer), name_chars),
-                       AllocationType::kOld)
-                   .ToHandleChecked();
-  }
-  script->set_name(*name_str);
-  MaybeHandle<String> url_str;
-  if (!source_url.empty()) {
-    url_str =
-        isolate->factory()->NewStringFromUtf8(source_url, AllocationType::kOld);
-  } else {
-    Handle<String> url_prefix =
-        isolate->factory()->InternalizeString(StaticCharVector("wasm://wasm/"));
-    url_str = isolate->factory()->NewConsString(url_prefix, name_str);
-  }
-  script->set_source_url(*url_str.ToHandleChecked());
-
-  if (!source_map_url.empty()) {
-    MaybeHandle<String> src_map_str = isolate->factory()->NewStringFromUtf8(
-        source_map_url, AllocationType::kOld);
-    script->set_source_mapping_url(*src_map_str.ToHandleChecked());
-  }
-  return script;
-}
-
 }  // namespace wasm
 }  // namespace internal
 }  // namespace v8

src/wasm/module-compiler.h

@@ -60,11 +60,6 @@ WasmCode* CompileImportWrapper(
     compiler::WasmImportCallKind kind, const FunctionSig* sig,
     WasmImportWrapperCache::ModificationScope* cache_scope);
 
-V8_EXPORT_PRIVATE Handle<Script> CreateWasmScript(
-    Isolate* isolate, Vector<const uint8_t> wire_bytes,
-    Vector<const char> source_map_url, WireBytesRef name,
-    Vector<const char> source_url = {});
-
 // Triggered by the WasmCompileLazy builtin. The return value indicates whether
 // compilation was successful. Lazy compilation can fail only if validation is
 // also lazy.

src/wasm/wasm-engine.cc

@@ -489,10 +489,7 @@ MaybeHandle<WasmModuleObject> WasmEngine::SyncCompile(
   }
 #endif
 
-  Handle<Script> script =
-      GetOrCreateScript(isolate, native_module.get(),
-                        VectorOf(native_module->module()->source_map_url),
-                        native_module->module()->name);
+  Handle<Script> script = GetOrCreateScript(isolate, native_module);
 
   // Create the compiled module object and populate with compiled functions
   // and information needed at instantiation time. This object needs to be
@@ -656,19 +653,97 @@ std::shared_ptr<NativeModule> WasmEngine::ExportNativeModule(
   return module_object->shared_native_module();
 }
 
+namespace {
+Handle<Script> CreateWasmScript(Isolate* isolate,
+                                std::shared_ptr<NativeModule> native_module,
+                                Vector<const char> source_url = {}) {
+  Handle<Script> script =
+      isolate->factory()->NewScript(isolate->factory()->empty_string());
+  script->set_compilation_state(Script::COMPILATION_STATE_COMPILED);
+  script->set_context_data(isolate->native_context()->debug_context_id());
+  script->set_type(Script::TYPE_WASM);
+
+  Vector<const uint8_t> wire_bytes = native_module->wire_bytes();
+  int hash = StringHasher::HashSequentialString(
+      reinterpret_cast<const char*>(wire_bytes.begin()), wire_bytes.length(),
+      kZeroHashSeed);
+
+  const int kBufferSize = 32;
+  char buffer[kBufferSize];
+
+  // Script name is "<module_name>-hash" if name is available and "hash"
+  // otherwise.
+  const WasmModule* module = native_module->module();
+  Handle<String> name_str;
+  if (module->name.is_set()) {
+    int name_chars = SNPrintF(ArrayVector(buffer), "-%08x", hash);
+    DCHECK(name_chars >= 0 && name_chars < kBufferSize);
+    Handle<String> name_hash =
+        isolate->factory()
+            ->NewStringFromOneByte(
+                VectorOf(reinterpret_cast<uint8_t*>(buffer), name_chars),
+                AllocationType::kOld)
+            .ToHandleChecked();
+    Handle<String> module_name =
+        WasmModuleObject::ExtractUtf8StringFromModuleBytes(
+            isolate, wire_bytes, module->name, kNoInternalize);
+    name_str = isolate->factory()
+                   ->NewConsString(module_name, name_hash)
+                   .ToHandleChecked();
+  } else {
+    int name_chars = SNPrintF(ArrayVector(buffer), "%08x", hash);
+    DCHECK(name_chars >= 0 && name_chars < kBufferSize);
+    name_str = isolate->factory()
+                   ->NewStringFromOneByte(
+                       VectorOf(reinterpret_cast<uint8_t*>(buffer), name_chars),
+                       AllocationType::kOld)
+                   .ToHandleChecked();
+  }
+  script->set_name(*name_str);
+  MaybeHandle<String> url_str;
+  if (!source_url.empty()) {
+    url_str =
+        isolate->factory()->NewStringFromUtf8(source_url, AllocationType::kOld);
+  } else {
+    Handle<String> url_prefix =
+        isolate->factory()->InternalizeString(StaticCharVector("wasm://wasm/"));
+    url_str = isolate->factory()->NewConsString(url_prefix, name_str);
+  }
+  script->set_source_url(*url_str.ToHandleChecked());
+
+  auto source_map_url = VectorOf(module->source_map_url);
+  if (!source_map_url.empty()) {
+    MaybeHandle<String> src_map_str = isolate->factory()->NewStringFromUtf8(
+        source_map_url, AllocationType::kOld);
+    script->set_source_mapping_url(*src_map_str.ToHandleChecked());
+  }
+
+  // Use the given shared {NativeModule}, but increase its reference count by
+  // allocating a new {Managed<T>} that the {Script} references.
+  size_t code_size_estimate = native_module->committed_code_space();
+  size_t memory_estimate =
+      code_size_estimate +
+      wasm::WasmCodeManager::EstimateNativeModuleMetaDataSize(module);
+  Handle<Managed<wasm::NativeModule>> managed_native_module =
+      Managed<wasm::NativeModule>::FromSharedPtr(isolate, memory_estimate,
+                                                 std::move(native_module));
+  script->set_wasm_managed_native_module(*managed_native_module);
+  script->set_wasm_breakpoint_infos(ReadOnlyRoots(isolate).empty_fixed_array());
+  script->set_wasm_weak_instance_list(
+      ReadOnlyRoots(isolate).empty_weak_array_list());
+  return script;
+}
+}  // namespace
+
 Handle<WasmModuleObject> WasmEngine::ImportNativeModule(
     Isolate* isolate, std::shared_ptr<NativeModule> shared_native_module) {
   NativeModule* native_module = shared_native_module.get();
   ModuleWireBytes wire_bytes(native_module->wire_bytes());
-  Handle<Script> script =
-      CreateWasmScript(isolate, wire_bytes.module_bytes(),
-                       VectorOf(native_module->module()->source_map_url),
-                       native_module->module()->name);
+  Handle<Script> script = GetOrCreateScript(isolate, shared_native_module);
   Handle<FixedArray> export_wrappers;
   CompileJsToWasmWrappers(isolate, native_module->module(), &export_wrappers);
   Handle<WasmModuleObject> module_object = WasmModuleObject::New(
-      isolate, std::move(shared_native_module), script, export_wrappers,
-      native_module->committed_code_space());
+      isolate, std::move(shared_native_module), script, export_wrappers);
   {
     base::MutexGuard lock(&mutex_);
     DCHECK_EQ(1, isolates_.count(isolate));
@@ -1118,16 +1193,14 @@ void WasmEngine::FreeDeadCodeLocked(const DeadCodeMap& dead_code) {
   }
 }
 
-Handle<Script> WasmEngine::GetOrCreateScript(Isolate* isolate,
-                                             NativeModule* native_module,
-                                             Vector<const char> source_map_url,
-                                             WireBytesRef name,
-                                             Vector<const char> source_url) {
+Handle<Script> WasmEngine::GetOrCreateScript(
+    Isolate* isolate, const std::shared_ptr<NativeModule>& native_module,
+    Vector<const char> source_url) {
   {
     base::MutexGuard guard(&mutex_);
     DCHECK_EQ(1, isolates_.count(isolate));
     auto& scripts = isolates_[isolate]->scripts;
-    auto it = scripts.find(native_module);
+    auto it = scripts.find(native_module.get());
     if (it != scripts.end()) {
       Handle<Script> weak_global_handle = it->second.handle();
       if (weak_global_handle.is_null()) {
@@ -1138,14 +1211,13 @@ Handle<Script> WasmEngine::GetOrCreateScript(Isolate* isolate,
     }
   }
   // Temporarily release the mutex to let the GC collect native modules.
-  auto script = CreateWasmScript(isolate, native_module->wire_bytes(),
-                                 source_map_url, name, source_url);
+  auto script = CreateWasmScript(isolate, native_module, source_url);
   {
     base::MutexGuard guard(&mutex_);
     DCHECK_EQ(1, isolates_.count(isolate));
     auto& scripts = isolates_[isolate]->scripts;
-    DCHECK_EQ(0, scripts.count(native_module));
-    scripts.emplace(native_module, WeakScriptHandle(script));
+    DCHECK_EQ(0, scripts.count(native_module.get()));
+    scripts.emplace(native_module.get(), WeakScriptHandle(script));
     return script;
   }
 }

src/wasm/wasm-engine.h

@@ -335,9 +335,8 @@ class V8_EXPORT_PRIVATE WasmEngine {
   void FreeDeadCode(const DeadCodeMap&);
   void FreeDeadCodeLocked(const DeadCodeMap&);
 
-  Handle<Script> GetOrCreateScript(Isolate*, NativeModule*,
-                                   Vector<const char> source_map_url,
-                                   WireBytesRef name,
+  Handle<Script> GetOrCreateScript(Isolate*,
+                                   const std::shared_ptr<NativeModule>&,
                                    Vector<const char> source_url = {});
 
   // Call on process start and exit.

src/wasm/wasm-objects.cc

@@ -165,43 +165,24 @@ Handle<WasmModuleObject> WasmModuleObject::New(
 Handle<WasmModuleObject> WasmModuleObject::New(
     Isolate* isolate, std::shared_ptr<wasm::NativeModule> native_module,
     Handle<Script> script, Handle<FixedArray> export_wrappers) {
-  const WasmModule* module = native_module->module();
-  const bool uses_liftoff =
-      FLAG_liftoff && native_module->module()->origin == wasm::kWasmOrigin;
-  size_t code_size_estimate =
-      wasm::WasmCodeManager::EstimateNativeModuleCodeSize(module, uses_liftoff);
-  return New(isolate, std::move(native_module), script, export_wrappers,
-             code_size_estimate);
-}
-
-// static
-Handle<WasmModuleObject> WasmModuleObject::New(
-    Isolate* isolate, std::shared_ptr<wasm::NativeModule> native_module,
-    Handle<Script> script, Handle<FixedArray> export_wrappers,
-    size_t code_size_estimate) {
-  const WasmModule* module = native_module->module();
-
-  // Use the given shared {NativeModule}, but increase its reference count by
-  // allocating a new {Managed<T>} that the {WasmModuleObject} references.
-  size_t memory_estimate =
-      code_size_estimate +
-      wasm::WasmCodeManager::EstimateNativeModuleMetaDataSize(module);
-  Handle<Managed<wasm::NativeModule>> managed_native_module =
-      Managed<wasm::NativeModule>::FromSharedPtr(isolate, memory_estimate,
-                                                 std::move(native_module));
-
+  Handle<Managed<wasm::NativeModule>> managed_native_module;
+  if (script->type() == Script::TYPE_WASM) {
+    managed_native_module = handle(
+        Managed<wasm::NativeModule>::cast(script->wasm_managed_native_module()),
+        isolate);
+  } else {
+    const WasmModule* module = native_module->module();
+    size_t memory_estimate =
+        native_module->committed_code_space() +
+        wasm::WasmCodeManager::EstimateNativeModuleMetaDataSize(module);
+    managed_native_module = Managed<wasm::NativeModule>::FromSharedPtr(
+        isolate, memory_estimate, std::move(native_module));
+  }
   Handle<WasmModuleObject> module_object = Handle<WasmModuleObject>::cast(
       isolate->factory()->NewJSObject(isolate->wasm_module_constructor()));
   module_object->set_export_wrappers(*export_wrappers);
-  if (script->type() == Script::TYPE_WASM) {
-    script->set_wasm_breakpoint_infos(
-        ReadOnlyRoots(isolate).empty_fixed_array());
-    script->set_wasm_managed_native_module(*managed_native_module);
-    script->set_wasm_weak_instance_list(
-        ReadOnlyRoots(isolate).empty_weak_array_list());
-  }
-  module_object->set_script(*script);
   module_object->set_managed_native_module(*managed_native_module);
+  module_object->set_script(*script);
   return module_object;
 }
 

src/wasm/wasm-objects.h

@@ -150,10 +150,6 @@ class WasmModuleObject : public JSObject {
   V8_EXPORT_PRIVATE static Handle<WasmModuleObject> New(
       Isolate* isolate, std::shared_ptr<wasm::NativeModule> native_module,
       Handle<Script> script, Handle<FixedArray> export_wrappers);
-  V8_EXPORT_PRIVATE static Handle<WasmModuleObject> New(
-      Isolate* isolate, std::shared_ptr<wasm::NativeModule> native_module,
-      Handle<Script> script, Handle<FixedArray> export_wrappers,
-      size_t code_size_estimate);
 
   // Check whether this module was generated from asm.js source.
   inline bool is_asm_js();

src/wasm/wasm-serialization.cc

@@ -611,9 +611,6 @@ MaybeHandle<WasmModuleObject> DeserializeNativeModule(
   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_vec,
-                                           VectorOf(module->source_map_url),
-                                           module->name, source_url);
 
   auto shared_native_module = wasm_engine->MaybeGetNativeModule(
       module->origin, wire_bytes_vec, isolate);
@@ -641,6 +638,8 @@ MaybeHandle<WasmModuleObject> DeserializeNativeModule(
   CompileJsToWasmWrappers(isolate, shared_native_module->module(),
                           &export_wrappers);
 
+  Handle<Script> script =
+      wasm_engine->GetOrCreateScript(isolate, shared_native_module);
   Handle<WasmModuleObject> module_object = WasmModuleObject::New(
       isolate, std::move(shared_native_module), script, export_wrappers);
 

test/cctest/wasm/wasm-run-utils.cc

@@ -364,10 +364,6 @@ const WasmGlobal* TestingModuleBuilder::AddGlobal(ValueType type) {
 }
 
 Handle<WasmInstanceObject> TestingModuleBuilder::InitInstanceObject() {
-  Handle<Script> script =
-      isolate_->factory()->NewScript(isolate_->factory()->empty_string());
-  script->set_type(Script::TYPE_WASM);
-
   const bool kUsesLiftoff = true;
   size_t code_size_estimate =
       wasm::WasmCodeManager::EstimateNativeModuleCodeSize(test_module_.get(),
@@ -375,6 +371,8 @@ Handle<WasmInstanceObject> TestingModuleBuilder::InitInstanceObject() {
   auto native_module = isolate_->wasm_engine()->NewNativeModule(
       isolate_, enabled_features_, test_module_, code_size_estimate);
   native_module->SetWireBytes(OwnedVector<const uint8_t>());
+  Handle<Script> script =
+      isolate_->wasm_engine()->GetOrCreateScript(isolate_, native_module);
 
   Handle<WasmModuleObject> module_object =
       WasmModuleObject::New(isolate_, std::move(native_module), script);

test/mjsunit/wasm/compiled-module-management.js

@@ -80,6 +80,7 @@ assertEquals(0, %WasmGetNumberOfInstances(module));
   instance4 = new WebAssembly.Instance(module, {"": {getValue: () => 4}});
   assertEquals(4, instance4.exports.f());
   module = null;
+  instance4 = null;
 })();
 
 // Note that two GC's are required because weak slots clearing is deferred.