Commit 64d0ceb3 authored by Adam Klein's avatar Adam Klein Committed by V8 LUCI CQ

Revert "[wasm] Move write scope out of NativeModule::AddCode"

This reverts commit 6ae18c2d.

Reason for revert: breaks a bunch of tests on Mac arm64 bots:

https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Mac%20-%20arm64%20-%20release/5754/overview
https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Mac%20-%20arm64%20-%20debug/2421/overview

Original change's description:
> [wasm] Move write scope out of NativeModule::AddCode
>
> {NativeModule::AddCode} is a central method that should usually be
> called in batches, where the caller holds a {CodeSpaceWriteScope} for a
> longer time (over several compilations).
> This CL moves us closer to that by removing the scope from that central
> method and instead putting it in callers where it becomes more visible.
> There are already TODOs to introduce caching or batching to avoid some
> switching, and one more TODO is added.
>
> Drive-by: Remove an unneeded {CodeSpaceMemoryModificationScope}.
>
> R=​jkummerow@chromium.org
>
> Bug: v8:11974
> Change-Id: Ia13c601abc766e5fca6ca053bf1fc4d647b53ed0
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3098186
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#76344}

Bug: v8:11974
Change-Id: Ia6a6814f153f7602d5d691bc5c930601ff4622a7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3111268
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Adam Klein <adamk@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76414}
parent 5e47fccd
......@@ -3658,17 +3658,13 @@ WasmCode* CompileImportWrapper(
CompilationEnv env = native_module->CreateCompilationEnv();
WasmCompilationResult result = compiler::CompileWasmImportCallWrapper(
&env, kind, sig, source_positions, expected_arity);
WasmCode* published_code;
{
CodeSpaceWriteScope code_space_write_scope(native_module);
std::unique_ptr<WasmCode> wasm_code = native_module->AddCode(
result.func_index, result.code_desc, result.frame_slot_count,
result.tagged_parameter_slots,
result.protected_instructions_data.as_vector(),
result.source_positions.as_vector(), GetCodeKind(result),
ExecutionTier::kNone, kNoDebugging);
published_code = native_module->PublishCode(std::move(wasm_code));
}
std::unique_ptr<WasmCode> wasm_code = native_module->AddCode(
result.func_index, result.code_desc, result.frame_slot_count,
result.tagged_parameter_slots,
result.protected_instructions_data.as_vector(),
result.source_positions.as_vector(), GetCodeKind(result),
ExecutionTier::kNone, kNoDebugging);
WasmCode* published_code = native_module->PublishCode(std::move(wasm_code));
(*cache_scope)[key] = published_code;
published_code->IncRef();
counters->wasm_generated_code_size()->Increment(
......
......@@ -65,10 +65,9 @@ class CompileImportWrapperJob final : public JobTask {
}
void Run(JobDelegate* delegate) override {
CodeSpaceWriteScope code_space_write_scope(native_module_);
while (base::Optional<WasmImportWrapperCache::CacheKey> key =
queue_->pop()) {
// TODO(wasm): Batch code publishing, to avoid repeated locking and
// permission switching.
CompileImportWrapper(native_module_, counters_, key->kind, key->signature,
key->expected_arity, cache_scope_);
if (delegate->ShouldYield()) return;
......
......@@ -1179,6 +1179,7 @@ std::unique_ptr<WasmCode> NativeModule::AddCode(
ExecutionTier tier, ForDebugging for_debugging) {
base::Vector<byte> code_space;
NativeModule::JumpTablesRef jump_table_ref;
CodeSpaceWriteScope code_space_write_scope(this);
{
base::RecursiveMutexGuard guard{&allocation_mutex_};
code_space = code_allocator_.AllocateForCode(this, desc.instr_size);
......
......@@ -17,7 +17,6 @@
#include "src/objects/struct-inl.h"
#include "src/trap-handler/trap-handler.h"
#include "src/utils/utils.h"
#include "src/wasm/code-space-access.h"
#include "src/wasm/jump-table-assembler.h"
#include "src/wasm/module-compiler.h"
#include "src/wasm/module-decoder.h"
......@@ -1533,8 +1532,7 @@ void WasmInstanceObject::ImportWasmJSFunctionIntoTable(
if (sig_id >= 0) {
wasm::NativeModule* native_module =
instance->module_object().native_module();
// TODO(wasm): Cache and reuse wrapper code, to avoid repeated compilation
// and permissions switching.
// TODO(wasm): Cache and reuse wrapper code.
const wasm::WasmFeatures enabled = native_module->enabled_features();
auto resolved = compiler::ResolveWasmImportCall(
callable, sig, instance->module(), enabled);
......@@ -1551,7 +1549,6 @@ void WasmInstanceObject::ImportWasmJSFunctionIntoTable(
}
wasm::WasmCompilationResult result = compiler::CompileWasmImportCallWrapper(
&env, kind, sig, false, expected_arity);
wasm::CodeSpaceWriteScope write_scope(native_module);
std::unique_ptr<wasm::WasmCode> wasm_code = native_module->AddCode(
result.func_index, result.code_desc, result.frame_slot_count,
result.tagged_parameter_slots,
......
......@@ -11,7 +11,6 @@
#include "src/base/platform/elapsed-timer.h"
#include "src/codegen/assembler-inl.h"
#include "src/utils/utils.h"
#include "src/wasm/code-space-access.h"
#include "src/wasm/wasm-opcodes-inl.h"
#include "test/cctest/cctest.h"
#include "test/cctest/compiler/value-helper.h"
......@@ -3881,31 +3880,28 @@ TEST(Liftoff_tier_up) {
r.builder().instance_object()->module_object().native_module();
// This test only works if we managed to compile with Liftoff.
if (!native_module->GetCode(add.function_index())->is_liftoff()) return;
// First run should execute {add}.
CHECK_EQ(18, r.Call(11, 7));
// Now make a copy of the {sub} function, and add it to the native module at
// the index of {add}.
CodeDesc desc;
memset(&desc, 0, sizeof(CodeDesc));
WasmCode* sub_code = native_module->GetCode(sub.function_index());
size_t sub_size = sub_code->instructions().size();
std::unique_ptr<byte[]> buffer(new byte[sub_code->instructions().size()]);
memcpy(buffer.get(), sub_code->instructions().begin(), sub_size);
desc.buffer = buffer.get();
desc.instr_size = static_cast<int>(sub_size);
{
CodeSpaceWriteScope write_scope(native_module);
if (native_module->GetCode(add.function_index())->is_liftoff()) {
// First run should execute {add}.
CHECK_EQ(18, r.Call(11, 7));
// Now make a copy of the {sub} function, and add it to the native module at
// the index of {add}.
CodeDesc desc;
memset(&desc, 0, sizeof(CodeDesc));
WasmCode* sub_code = native_module->GetCode(sub.function_index());
size_t sub_size = sub_code->instructions().size();
std::unique_ptr<byte[]> buffer(new byte[sub_code->instructions().size()]);
memcpy(buffer.get(), sub_code->instructions().begin(), sub_size);
desc.buffer = buffer.get();
desc.instr_size = static_cast<int>(sub_size);
std::unique_ptr<WasmCode> new_code = native_module->AddCode(
add.function_index(), desc, 0, 0, {}, {}, WasmCode::kFunction,
ExecutionTier::kTurbofan, kNoDebugging);
native_module->PublishCode(std::move(new_code));
}
// Second run should now execute {sub}.
CHECK_EQ(4, r.Call(11, 7));
// Second run should now execute {sub}.
CHECK_EQ(4, r.Call(11, 7));
}
}
TEST(Regression_1085507) {
......
......@@ -9,7 +9,6 @@
#include "src/diagnostics/code-tracer.h"
#include "src/heap/heap-inl.h"
#include "src/wasm/baseline/liftoff-compiler.h"
#include "src/wasm/code-space-access.h"
#include "src/wasm/graph-builder-interface.h"
#include "src/wasm/leb-helper.h"
#include "src/wasm/module-compiler.h"
......@@ -70,6 +69,7 @@ TestingModuleBuilder::TestingModuleBuilder(
if (maybe_import) {
// Manually compile an import wrapper and insert it into the instance.
CodeSpaceMemoryModificationScope modification_scope(isolate_->heap());
auto resolved = compiler::ResolveWasmImportCall(
maybe_import->js_function, maybe_import->sig,
instance_object_->module(), enabled_features_);
......@@ -82,7 +82,6 @@ TestingModuleBuilder::TestingModuleBuilder(
static_cast<int>(maybe_import->sig->parameter_count()));
auto import_wrapper = cache_scope[key];
if (import_wrapper == nullptr) {
CodeSpaceWriteScope write_scope(native_module_);
import_wrapper = CompileImportWrapper(
native_module_, isolate_->counters(), kind, maybe_import->sig,
static_cast<int>(maybe_import->sig->parameter_count()), &cache_scope);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment