[wasm] Split adding code from publishing it

This prepares a refactoring to add and publish compilation results in
batches. For this, we need to separate the two phases, so that we can
lock the module, allocate all the code space, release the lock, copy
the code, lock the module, publish the code, and release the lock
again.
In particular, this CL does the following:
1) It removes the {AddOwnedCode} method. The functionality of creating
   the {WasmCode} and memcpy'ing the instruction into that is done in
   the other {Add*Code} methods. Adding to {owned_code_} is done in
   {PublishCode}.
2) {PublishInterpreterEntry} is now functionally equivalent to
   {PublishCode}, so it's removed.
3) After {AddCode}, the caller has to call {PublishCode}. In a
   follow-up CL, this will be called in batches (first {AddCode} them
   all, then {PublishCode} them all).
4) {AddCompiledCode} now assumes that the {WasmCompilationResult}
   succeeded. Otherwise, the caller should directly call {SetError} on
   the {CompilationState}.
5) {PublishCode} is now the chokepoint for installing code to the code
   table, the owned code vector, the jump table, and setting interpreter
   redirections. It replaces previous direct calls to {InstallCode} or
   explicitly adding to {owned_code_}.
6) Increasing the {generated_code_size_} counter is now done in
   {AllocateForCode}, which is the chokepoint for allocating space for
   generated code. This way, we will only increase this counter once
   once we allocate in batches.

R=titzer@chromium.org

Bug: v8:8916
Change-Id: I71e02e3a838f21797915cee3ebd373804fb12237
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1530817
Commit-Queue: Clemens Hammacher <clemensh@chromium.org>
Reviewed-by: Ben Titzer <titzer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60369}

src/compiler/wasm-compiler.cc

@@ -5882,16 +5882,14 @@ wasm::WasmCode* CompileWasmMathIntrinsic(wasm::WasmEngine* wasm_engine,
       wasm_engine, call_descriptor, mcgraph, Code::WASM_FUNCTION,
       wasm::WasmCode::kFunction, debug_name, WasmStubAssemblerOptions(),
       source_positions);
-  wasm::WasmCode* wasm_code = native_module->AddCode(
+  std::unique_ptr<wasm::WasmCode> wasm_code = native_module->AddCode(
       wasm::WasmCode::kAnonymousFuncIndex, result.code_desc,
       result.frame_slot_count, result.tagged_parameter_slots,
       std::move(result.protected_instructions),
       std::move(result.source_positions), wasm::WasmCode::kFunction,
       wasm::WasmCode::kOther);
-  CHECK_NOT_NULL(wasm_code);
   // TODO(titzer): add counters for math intrinsic code size / allocation
-
-  return wasm_code;
+  return native_module->PublishCode(std::move(wasm_code));
 }
 
 wasm::WasmCode* CompileWasmImportCallWrapper(wasm::WasmEngine* wasm_engine,
@@ -5949,16 +5947,13 @@ wasm::WasmCode* CompileWasmImportCallWrapper(wasm::WasmEngine* wasm_engine,
       wasm_engine, incoming, &jsgraph, Code::WASM_TO_JS_FUNCTION,
       wasm::WasmCode::kWasmToJsWrapper, func_name, WasmStubAssemblerOptions(),
       source_position_table);
-  wasm::WasmCode* wasm_code = native_module->AddCode(
+  std::unique_ptr<wasm::WasmCode> wasm_code = native_module->AddCode(
       wasm::WasmCode::kAnonymousFuncIndex, result.code_desc,
       result.frame_slot_count, result.tagged_parameter_slots,
       std::move(result.protected_instructions),
       std::move(result.source_positions), wasm::WasmCode::kWasmToJsWrapper,
       wasm::WasmCode::kOther);
-
-  CHECK_NOT_NULL(wasm_code);
-
-  return wasm_code;
+  return native_module->PublishCode(std::move(wasm_code));
 }
 
 wasm::WasmCompilationResult CompileWasmInterpreterEntry(

src/wasm/function-compiler.cc

@@ -230,7 +230,11 @@ void WasmCompilationUnit::CompileWasmFunction(Isolate* isolate,
   WasmCompilationResult result = unit.ExecuteCompilation(
       &env, native_module->compilation_state()->GetWireBytesStorage(),
       isolate->counters(), detected);
-  native_module->AddCompiledCode(std::move(result));
+  if (result.succeeded()) {
+    native_module->AddCompiledCode(std::move(result));
+  } else {
+    native_module->compilation_state()->SetError();
+  }
 }
 
 }  // namespace wasm

src/wasm/module-compiler.cc

@@ -482,8 +482,12 @@ bool FetchAndExecuteCompilationUnit(CompilationEnv* env,
   WasmCompilationResult result = unit->ExecuteCompilation(
       env, compilation_state->GetWireBytesStorage(), counters, detected);
 
-  WasmCode* code = native_module->AddCompiledCode(std::move(result));
-  compilation_state->OnFinishedUnit(result.requested_tier, code);
+  if (result.succeeded()) {
+    WasmCode* code = native_module->AddCompiledCode(std::move(result));
+    compilation_state->OnFinishedUnit(result.requested_tier, code);
+  } else {
+    compilation_state->SetError();
+  }
 
   return true;
 }
@@ -702,15 +706,17 @@ class BackgroundCompileTask : public CancelableTask {
       {
         BackgroundCompileScope compile_scope(token_);
         if (compile_scope.cancelled()) return;
-        WasmCode* code =
-            compile_scope.native_module()->AddCompiledCode(std::move(result));
-        if (code == nullptr) {
+        if (!result.succeeded()) {
           // Compile error.
+          compile_scope.compilation_state()->SetError();
           compile_scope.compilation_state()->OnBackgroundTaskStopped(
               detected_features);
           compilation_failed = true;
           break;
         }
+        WasmCode* code =
+            compile_scope.native_module()->AddCompiledCode(std::move(result));
+        DCHECK_NOT_NULL(code);
 
         // Successfully finished one unit.
         compile_scope.compilation_state()->OnFinishedUnit(result.requested_tier,

src/wasm/wasm-code-manager.h

@@ -232,14 +232,19 @@ class V8_EXPORT_PRIVATE NativeModule final {
 
   // {AddCode} is thread safe w.r.t. other calls to {AddCode} or methods adding
   // code below, i.e. it can be called concurrently from background threads.
-  // {AddCode} also makes the code available to the system by entering it into
-  // the code table and patching the jump table.
-  WasmCode* AddCode(uint32_t index, const CodeDesc& desc, uint32_t stack_slots,
-                    uint32_t tagged_parameter_slots,
-                    OwnedVector<trap_handler::ProtectedInstructionData>
-                        protected_instructions,
-                    OwnedVector<const byte> source_position_table,
-                    WasmCode::Kind kind, WasmCode::Tier tier);
+  // The returned code still needs to be published via {PublishCode}.
+  std::unique_ptr<WasmCode> AddCode(
+      uint32_t index, const CodeDesc& desc, uint32_t stack_slots,
+      uint32_t tagged_parameter_slots,
+      OwnedVector<trap_handler::ProtectedInstructionData>
+          protected_instructions,
+      OwnedVector<const byte> source_position_table, WasmCode::Kind kind,
+      WasmCode::Tier tier);
+
+  // {PublishCode} makes the code available to the system by entering it into
+  // the code table and patching the jump table. It returns a raw pointer to the
+  // given {WasmCode} object.
+  WasmCode* PublishCode(std::unique_ptr<WasmCode>);
 
   WasmCode* AddDeserializedCode(
       uint32_t index, Vector<const byte> instructions, uint32_t stack_slots,
@@ -265,11 +270,6 @@ class V8_EXPORT_PRIVATE NativeModule final {
   // other WasmCode so that runtime stub ids can be resolved during relocation.
   void SetRuntimeStubs(Isolate* isolate);
 
-  // Switch a function to an interpreter entry wrapper. When adding interpreter
-  // wrappers, we do not insert them in the code_table, however, we let them
-  // self-identify as the {index} function.
-  void PublishInterpreterEntry(WasmCode* code, uint32_t index);
-
   // Creates a snapshot of the current state of the code table. This is useful
   // to get a consistent view of the table (e.g. used by the serializer).
   std::vector<WasmCode*> SnapshotCodeTable() const;
@@ -380,32 +380,14 @@ class V8_EXPORT_PRIVATE NativeModule final {
                std::shared_ptr<Counters> async_counters,
                std::shared_ptr<NativeModule>* shared_this);
 
-  WasmCode* AddAnonymousCode(Handle<Code>, WasmCode::Kind kind,
-                             const char* name = nullptr);
+  // Add and publish anonymous code.
+  WasmCode* AddAndPublishAnonymousCode(Handle<Code>, WasmCode::Kind kind,
+                                       const char* name = nullptr);
   // Allocate code space. Returns a valid buffer or fails with OOM (crash).
   Vector<byte> AllocateForCode(size_t size);
 
-  // Primitive for adding code to the native module. All code added to a native
-  // module is owned by that module. Various callers get to decide on how the
-  // code is obtained (CodeDesc vs, as a point in time, Code), the kind,
-  // whether it has an index or is anonymous, etc.
-  WasmCode* AddOwnedCode(uint32_t index, Vector<const byte> instructions,
-                         uint32_t stack_slots, uint32_t tagged_parameter_slots,
-                         size_t safepoint_table_offset,
-                         size_t handler_table_offset,
-                         size_t constant_pool_offset,
-                         size_t code_comments_offset,
-                         size_t unpadded_binary_size,
-                         OwnedVector<trap_handler::ProtectedInstructionData>,
-                         OwnedVector<const byte> reloc_info,
-                         OwnedVector<const byte> source_position_table,
-                         WasmCode::Kind, WasmCode::Tier);
-
   WasmCode* CreateEmptyJumpTable(uint32_t jump_table_size);
 
-  // Hold the {allocation_mutex_} when calling this method.
-  void InstallCode(WasmCode* code);
-
   Vector<WasmCode*> code_table() const {
     return {code_table_.get(), module_->num_declared_functions};
   }

src/wasm/wasm-debug.cc

@@ -583,20 +583,16 @@ void WasmDebugInfo::RedirectToInterpreter(Handle<WasmDebugInfo> debug_info,
     wasm::WasmCompilationResult result = compiler::CompileWasmInterpreterEntry(
         isolate->wasm_engine(), native_module->enabled_features(), func_index,
         module->functions[func_index].sig);
-    // The interpreter entry is added with {kAnonymousFuncIndex} so that it will
-    // not replace existing code in the code table, only the jump table is
-    // redirected by {PublishInterpreterEntry}.
-    wasm::WasmCode* wasm_code = native_module->AddCode(
-        wasm::WasmCode::kAnonymousFuncIndex, result.code_desc,
-        result.frame_slot_count, result.tagged_parameter_slots,
-        std::move(result.protected_instructions),
+    std::unique_ptr<wasm::WasmCode> wasm_code = native_module->AddCode(
+        func_index, result.code_desc, result.frame_slot_count,
+        result.tagged_parameter_slots, std::move(result.protected_instructions),
         std::move(result.source_positions), wasm::WasmCode::kInterpreterEntry,
         wasm::WasmCode::kOther);
-    DCHECK_NOT_NULL(wasm_code);
-    native_module->PublishInterpreterEntry(wasm_code, func_index);
+    Address instruction_start = wasm_code->instruction_start();
+    native_module->PublishCode(std::move(wasm_code));
 
-    Handle<Foreign> foreign_holder = isolate->factory()->NewForeign(
-        wasm_code->instruction_start(), AllocationType::kOld);
+    Handle<Foreign> foreign_holder =
+        isolate->factory()->NewForeign(instruction_start, AllocationType::kOld);
     interpreted_functions->set(func_index, *foreign_holder);
   }
 }

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

@@ -3725,9 +3725,10 @@ TEST(Liftoff_tier_up) {
     memcpy(buffer.get(), sub_code->instructions().start(), sub_size);
     desc.buffer = buffer.get();
     desc.instr_size = static_cast<int>(sub_size);
-    native_module->AddCode(add.function_index(), desc, 0, 0, {},
-                           OwnedVector<byte>(), WasmCode::kFunction,
-                           WasmCode::kOther);
+    std::unique_ptr<WasmCode> new_code = native_module->AddCode(
+        add.function_index(), desc, 0, 0, {}, OwnedVector<byte>(),
+        WasmCode::kFunction, WasmCode::kOther);
+    native_module->PublishCode(std::move(new_code));
 
     // Second run should now execute {sub}.
     CHECK_EQ(4, r.Call(11, 7));

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

@@ -153,13 +153,12 @@ Handle<JSFunction> TestingModuleBuilder::WrapCode(uint32_t index) {
     wasm::WasmCompilationResult result = compiler::CompileWasmInterpreterEntry(
         isolate_->wasm_engine(), native_module_->enabled_features(), index,
         sig);
-    wasm::WasmCode* wasm_new_code = native_module_->AddCode(
-        wasm::WasmCode::kAnonymousFuncIndex, result.code_desc,
-        result.frame_slot_count, result.tagged_parameter_slots,
-        std::move(result.protected_instructions),
+    std::unique_ptr<wasm::WasmCode> code = native_module_->AddCode(
+        index, result.code_desc, result.frame_slot_count,
+        result.tagged_parameter_slots, std::move(result.protected_instructions),
         std::move(result.source_positions), wasm::WasmCode::kInterpreterEntry,
         wasm::WasmCode::kOther);
-    native_module_->PublishInterpreterEntry(wasm_new_code, index);
+    native_module_->PublishCode(std::move(code));
   }
   return ret;
 }

test/unittests/wasm/wasm-code-manager-unittest.cc

@@ -175,8 +175,9 @@ class WasmCodeManagerTest : public TestWithContext,
     std::unique_ptr<byte[]> exec_buff(new byte[size]);
     desc.buffer = exec_buff.get();
     desc.instr_size = static_cast<int>(size);
-    return native_module->AddCode(index, desc, 0, 0, {}, OwnedVector<byte>(),
-                                  WasmCode::kFunction, WasmCode::kOther);
+    std::unique_ptr<WasmCode> code = native_module->AddCode(
+        index, desc, 0, 0, {}, {}, WasmCode::kFunction, WasmCode::kOther);
+    return native_module->PublishCode(std::move(code));
   }
 
   size_t page() const { return AllocatePageSize(); }