From 3f366186e986f715d61a179fa223e2209b6fe882 Mon Sep 17 00:00:00 2001 From: littledan Date: Mon, 19 Sep 2016 12:29:58 -0700 Subject: [PATCH] Revert of Fix async/await memory leak (patchset #5 id:160001 of https://codereview.chromium.org/2348403002/ ) Reason for revert: Still causes issues on bot (sometimes!) Original issue's description: > Reland of Fix async/await memory leak (patchset #1 id:1 of https://codereview.chromium.org/2354473002/ ) > > Reason for revert: > Relanding with faster-running test > > Original issue's description: > > Revert of Fix async/await memory leak (patchset #5 id:80001 of https://codereview.chromium.org/2334323006/ ) > > > > Reason for revert: > > newly introduced test async-await-loop times out: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20sim%20-%20MSAN/builds/10894/steps/Ignition%20-%20turbofan%20%28flakes%29/logs/async-await-loop > > > > Original issue's description: > > > Fix async/await memory leak > > > > > > This patch closes a memory leak in async/await where the desugaring > > > was creating a situation analagous to that described in v8:5002. > > > Intermediate Promises were being kept alive, so a long-running loop > > > would cause linear memory usage on the heap. This patch returns > > > undefined to the 'then' callback passed into PerformPromiseThen > > > in order to avoid this hazard. Test expectations are fixed to remove > > > expecting extraneous events which occurred on Promises that are > > > now not given unnecessarily complex resolution paths before being > > > thrown away. > > > > > > BUG=v8:5390 > > > > > > Committed: https://crrev.com/a0ba18e9634c5e2d439033ab61a77cff54f9af35 > > > Cr-Commit-Position: refs/heads/master@{#39479} > > > > TBR=adamk@chromium.org,caitp@igalia.com,littledan@chromium.org > > NOTRY=true > > BUG=v8:5390 > > > > Committed: https://crrev.com/196db1999da130019bbf8e3bd65977f840e8afaf > > Cr-Commit-Position: refs/heads/master@{#39493} > > TBR=adamk@chromium.org,caitp@igalia.com,hablich@chromium.org > # Skipping CQ checks because original CL landed less than 1 days ago. > BUG=v8:5390 > > Committed: https://crrev.com/e51482f01f26e0013e6377e85c4d2c41900e403c > Cr-Commit-Position: refs/heads/master@{#39508} TBR=adamk@chromium.org,caitp@igalia.com,hablich@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=v8:5390 Review-Url: https://codereview.chromium.org/2348403003 Cr-Commit-Position: refs/heads/master@{#39512} --- src/js/harmony-async-await.js | 20 ++------- src/parsing/parser.cc | 42 ++++--------------- test/mjsunit/harmony/async-await-loop.js | 18 -------- .../mjsunit/harmony/async-await-throw-loop.js | 23 ---------- .../debug-async-function-async-task-event.js | 22 ++++++---- 5 files changed, 27 insertions(+), 98 deletions(-) delete mode 100644 test/mjsunit/harmony/async-await-loop.js delete mode 100644 test/mjsunit/harmony/async-await-throw-loop.js diff --git a/src/js/harmony-async-await.js b/src/js/harmony-async-await.js index dc82249e9fc4..396ebdc750a8 100644 --- a/src/js/harmony-async-await.js +++ b/src/js/harmony-async-await.js @@ -66,22 +66,10 @@ function AsyncFunctionAwait(generator, awaited, mark) { // ); var promise = PromiseCastResolved(awaited); - var onFulfilled = sentValue => { - %_Call(AsyncFunctionNext, generator, sentValue); - // The resulting Promise is a throwaway, so it doesn't matter what it - // resolves to. What is important is that we don't end up keeping the - // whole chain of intermediate Promises alive by returning the value - // of AsyncFunctionNext, as that would create a memory leak. - return; - }; - var onRejected = sentError => { - %_Call(AsyncFunctionThrow, generator, sentError); - // Similarly, returning the huge Promise here would cause a long - // resolution chain to find what the exception to throw is, and - // create a similar memory leak, and it does not matter what - // sort of rejection this intermediate Promise becomes. - return; - } + var onFulfilled = + (sentValue) => %_Call(AsyncFunctionNext, generator, sentValue); + var onRejected = + (sentError) => %_Call(AsyncFunctionThrow, generator, sentError); if (mark && DEBUG_IS_ACTIVE && IsPromise(awaited)) { // Mark the reject handler callback such that it does not influence diff --git a/src/parsing/parser.cc b/src/parsing/parser.cc index 49d63d518f25..56e3e0a7c89d 100644 --- a/src/parsing/parser.cc +++ b/src/parsing/parser.cc @@ -4560,46 +4560,21 @@ Expression* Parser::ExpressionListToExpression(ZoneList* args) { Expression* Parser::RewriteAwaitExpression(Expression* value, int await_pos) { // yield do { - // promise_tmp = .promise; // tmp = ; - // %AsyncFunctionAwait(.generator_object, tmp); - // promise_tmp + // tmp = %AsyncFunctionAwait(.generator_object, tmp); // } - // The value of the expression is returned to the caller of the async - // function for the first yield statement; for this, .promise is the - // appropriate return value, being a Promise that will be fulfilled or - // rejected with the appropriate value by the desugaring. Subsequent yield - // occurrences will return to the AsyncFunctionNext call within the - // implemementation of the intermediate throwaway Promise's then handler. - // This handler has nothing useful to do with the value, as the Promise is - // ignored. If we yielded the value of the throwawayPromise that - // AsyncFunctionAwait creates as an intermediate, it would create a memory - // leak; we must return .promise instead; - // The operand needs to be evaluated on a separate statement in order to get - // a break location, and the .promise needs to be read earlier so that it - // doesn't insert a false location. - // TODO(littledan): investigate why this ordering is needed in more detail. Variable* generator_object_variable = function_state_->generator_object_variable(); // If generator_object_variable is null, - // TODO(littledan): Is this necessary? if (!generator_object_variable) return value; const int nopos = kNoSourcePosition; - Block* do_block = factory()->NewBlock(nullptr, 3, false, nopos); - - Variable* promise_temp_var = - NewTemporary(ast_value_factory()->empty_string()); - Expression* promise_assignment = factory()->NewAssignment( - Token::ASSIGN, factory()->NewVariableProxy(promise_temp_var), - BuildDotPromise(), nopos); - do_block->statements()->Add( - factory()->NewExpressionStatement(promise_assignment, nopos), zone()); + Variable* temp_var = NewTemporary(ast_value_factory()->empty_string()); + Block* do_block = factory()->NewBlock(nullptr, 2, false, nopos); // Wrap value evaluation to provide a break location. - Variable* temp_var = NewTemporary(ast_value_factory()->empty_string()); Expression* value_assignment = factory()->NewAssignment( Token::ASSIGN, factory()->NewVariableProxy(temp_var), value, nopos); do_block->statements()->Add( @@ -4619,13 +4594,14 @@ Expression* Parser::RewriteAwaitExpression(Expression* value, int await_pos) { Expression* async_function_await = factory()->NewCallRuntime(Context::ASYNC_FUNCTION_AWAIT_CAUGHT_INDEX, async_function_await_args, nopos); - do_block->statements()->Add( - factory()->NewExpressionStatement(async_function_await, await_pos), - zone()); // Wrap await to provide a break location between value evaluation and yield. - Expression* do_expr = - factory()->NewDoExpression(do_block, promise_temp_var, nopos); + Expression* await_assignment = factory()->NewAssignment( + Token::ASSIGN, factory()->NewVariableProxy(temp_var), + async_function_await, nopos); + do_block->statements()->Add( + factory()->NewExpressionStatement(await_assignment, await_pos), zone()); + Expression* do_expr = factory()->NewDoExpression(do_block, temp_var, nopos); generator_object = factory()->NewVariableProxy(generator_object_variable); return factory()->NewYield(generator_object, do_expr, nopos, diff --git a/test/mjsunit/harmony/async-await-loop.js b/test/mjsunit/harmony/async-await-loop.js deleted file mode 100644 index 7b5f54b0a1ff..000000000000 --- a/test/mjsunit/harmony/async-await-loop.js +++ /dev/null @@ -1,18 +0,0 @@ -// Copyright 2016 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. - -// Flags: --harmony-async-await --allow-natives-syntax --max-old-space-size 3 - -let out; - -async function longLoop() { - for (let i = 0; i < 8000; i++) await undefined; - out = 1; -} - -longLoop(); - -%RunMicrotasks(); - -assertEquals(1, out); diff --git a/test/mjsunit/harmony/async-await-throw-loop.js b/test/mjsunit/harmony/async-await-throw-loop.js deleted file mode 100644 index 15ad09da1731..000000000000 --- a/test/mjsunit/harmony/async-await-throw-loop.js +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright 2016 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. - -// Flags: --harmony-async-await --allow-natives-syntax --max-old-space-size 3 - -let out; - -async function thrower() { throw undefined; } - -async function throwLoop() { - for (let i = 0; i < 8000; i++) { - try { await thrower(); } - catch (e) {} - } - out = 2; -} - -throwLoop(); - -%RunMicrotasks(); - -assertEquals(2, out); diff --git a/test/mjsunit/harmony/debug-async-function-async-task-event.js b/test/mjsunit/harmony/debug-async-function-async-task-event.js index 19c9c310dc93..249f02fc8f68 100644 --- a/test/mjsunit/harmony/debug-async-function-async-task-event.js +++ b/test/mjsunit/harmony/debug-async-function-async-task-event.js @@ -9,14 +9,20 @@ Debug = debug.Debug; var base_id = -1; var exception = null; var expected = [ - 'enqueue #1', - 'willHandle #1', - 'then #1', - 'enqueue #2', - 'didHandle #1', - 'willHandle #2', - 'then #2', - 'didHandle #2', + "enqueue #1", + "willHandle #1", + "then #1", + "enqueue #2", + "enqueue #3", + "didHandle #1", + "willHandle #2", + "then #2", + "didHandle #2", + "willHandle #3", + "enqueue #4", + "didHandle #3", + "willHandle #4", + "didHandle #4", ]; function assertLog(msg) {