Commit 2919e543 authored by Clemens Backes's avatar Clemens Backes Committed by Commit Bot

Revert "[wasm][debug] Garbage-collect stepping code"

This reverts commit 0938188f.

Reason for revert: new test times out on tsan: https://ci.chromium.org/ui/p/v8/builders/ci/V8%20Linux64%20TSAN/35152/overview

Original change's description:
> [wasm][debug] Garbage-collect stepping code
>
> All wasm code has an initial ref count of 1, in the expectation that it
> will be added to the code table. When the code is removed from that
> table, the ref count will be decremented.
> Stepping code (and also other code under special circumstances) will not
> be added to the code table though. Hence the ref count will never be
> decremented below 1, and the code will never be garbage-collected.
>
> This CL fixes this, by decrementing the ref count if the code is not
> added to the code table.
> Note that the code will only be collected if no isolate is currently
> using it, so it won't be collected while still in use for stepping.
>
> R=​thibaudm@chromium.org
>
> Bug: chromium:1168564
> Change-Id: I3047753591cbc52689ca019e9548ec58c237b835
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2649040
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Reviewed-by: Thibaud Michaud <thibaudm@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72354}

TBR=clemensb@chromium.org,thibaudm@chromium.org

Change-Id: I84f84324d2c4a3cae2ae6b97f469e3f22b0e3b3f
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: chromium:1168564
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2652485Reviewed-by: 's avatarClemens Backes <clemensb@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72357}
parent 08bb94e9
...@@ -1133,10 +1133,6 @@ WasmCode* NativeModule::PublishCodeLocked(std::unique_ptr<WasmCode> code) { ...@@ -1133,10 +1133,6 @@ WasmCode* NativeModule::PublishCodeLocked(std::unique_ptr<WasmCode> code) {
// The caller must hold the {allocation_mutex_}, thus we fail to lock it here. // The caller must hold the {allocation_mutex_}, thus we fail to lock it here.
DCHECK(!allocation_mutex_.TryLock()); DCHECK(!allocation_mutex_.TryLock());
// Add the code to the surrounding code ref scope, so the returned pointer is
// guaranteed to be valid.
WasmCodeRefScope::AddRef(code.get());
if (!code->IsAnonymous() && if (!code->IsAnonymous() &&
code->index() >= module_->num_imported_functions) { code->index() >= module_->num_imported_functions) {
DCHECK_LT(code->index(), num_functions()); DCHECK_LT(code->index(), num_functions());
...@@ -1173,21 +1169,17 @@ WasmCode* NativeModule::PublishCodeLocked(std::unique_ptr<WasmCode> code) { ...@@ -1173,21 +1169,17 @@ WasmCode* NativeModule::PublishCodeLocked(std::unique_ptr<WasmCode> code) {
WasmCodeRefScope::AddRef(prior_code); WasmCodeRefScope::AddRef(prior_code);
// The code is added to the current {WasmCodeRefScope}, hence the ref // The code is added to the current {WasmCodeRefScope}, hence the ref
// count cannot drop to zero here. // count cannot drop to zero here.
prior_code->DecRefOnLiveCode(); CHECK(!prior_code->DecRef());
} }
PatchJumpTablesLocked(slot_idx, code->instruction_start()); PatchJumpTablesLocked(slot_idx, code->instruction_start());
} else {
// The code tables does not hold a reference to the code, hence decrement
// the initial ref count of 1. The code was added to the
// {WasmCodeRefScope} though, so it cannot die here.
code->DecRefOnLiveCode();
} }
if (!code->for_debugging() && tiering_state_ == kTieredDown && if (!code->for_debugging() && tiering_state_ == kTieredDown &&
code->tier() == ExecutionTier::kTurbofan) { code->tier() == ExecutionTier::kTurbofan) {
liftoff_bailout_count_.fetch_add(1); liftoff_bailout_count_.fetch_add(1);
} }
} }
WasmCodeRefScope::AddRef(code.get());
WasmCode* result = code.get(); WasmCode* result = code.get();
owned_code_.emplace(result->instruction_start(), std::move(code)); owned_code_.emplace(result->instruction_start(), std::move(code));
return result; return result;
......
...@@ -228,14 +228,6 @@ class V8_EXPORT_PRIVATE WasmCode final { ...@@ -228,14 +228,6 @@ class V8_EXPORT_PRIVATE WasmCode final {
} }
} }
// Decrement the ref count on code that is known to be in use (i.e. the ref
// count cannot drop to zero here).
void DecRefOnLiveCode() {
int old_count = ref_count_.fetch_sub(1, std::memory_order_acq_rel);
DCHECK_LE(2, old_count);
USE(old_count);
}
// Decrement the ref count on code that is known to be dead, even though there // Decrement the ref count on code that is known to be dead, even though there
// might still be C++ references. Returns whether this drops the last // might still be C++ references. Returns whether this drops the last
// reference and the code needs to be freed. // reference and the code needs to be freed.
......
Tests repeated stepping through a large function (should not OOM)
Running test: test
Setting up global instance variable.
Got wasm script: wasm://wasm/46f40116
Setting breakpoint
Paused 500 and running...
Paused 1000 and running...
Paused 1500 and running...
Paused 2000 and running...
Paused 2500 and running...
Paused 3000 and running...
Paused 3500 and running...
Paused 4000 and running...
test function returned.
Paused 4003 times.
// Copyright 2021 the V8 project authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
utils.load('test/inspector/wasm-inspector-test.js');
const {session, contextGroup, Protocol} = InspectorTest.start(
'Tests repeated stepping through a large function (should not OOM)');
session.setupScriptMap();
const builder = new WasmModuleBuilder();
const body = [kExprLocalGet, 0];
// Stepping through a long function will repeatedly recreate stepping code, with
// corresponding side tables. This should not run OOM
// (https://crbug.com/1168564).
for (let i = 0; i < 2000; ++i) body.push(...wasmI32Const(i), kExprI32Add);
const func_test =
builder.addFunction('test', kSig_i_i).addBody(body).exportFunc();
const module_bytes = builder.toArray();
let paused = 0;
Protocol.Debugger.onPaused(msg => {
++paused;
if (paused % 500 == 0) InspectorTest.log(`Paused ${paused} and running...`);
Protocol.Debugger.stepOver();
});
InspectorTest.runAsyncTestSuite([
async function test() {
await Protocol.Debugger.enable();
InspectorTest.log('Setting up global instance variable.');
WasmInspectorTest.instantiate(module_bytes);
const [, {params: wasmScript}] = await Protocol.Debugger.onceScriptParsed(2);
InspectorTest.log('Got wasm script: ' + wasmScript.url);
InspectorTest.log('Setting breakpoint');
await Protocol.Debugger.setBreakpoint({
location: {
scriptId: wasmScript.scriptId,
lineNumber: 0,
columnNumber: func_test.body_offset
}
});
await Protocol.Runtime.evaluate({ expression: 'instance.exports.test()' });
InspectorTest.log('test function returned.');
InspectorTest.log(`Paused ${paused} times.`);
}
]);
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