[JSC] do not reference AwaitExpression Promises in async function Promise chain
authorcaitp@igalia.com <caitp@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Nov 2016 04:28:45 +0000 (04:28 +0000)
committercaitp@igalia.com <caitp@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Nov 2016 04:28:45 +0000 (04:28 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164753

Reviewed by Yusuke Suzuki.

JSTests:

* asyncFunctionTests.yaml:
* stress/async-await-long-loop.js: Added.
(shouldBe):
(async.longLoop):
* stress/async-await-throw-loop.js: Added.
(shouldBe):
(async.thrower):
(async.throwLoop):

Source/JavaScriptCore:

Previously, long-running async functions which contained many AwaitExpressions
would allocate and retain references to intermediate Promise objects for each `await`,
resulting in a memory leak.

To mitigate this leak, a reference to the original Promise (and its resolve and reject
functions) associated with the async function are kept, and passed to each call to
@asyncFunctionResume, while intermediate Promises are discarded. This is done by adding
a new Register to the BytecodeGenerator to hold the PromiseCapability object associated
with an async function wrapper. The capability is used to reject the Promise if an
exception is thrown during parameter initialization, and is used to store the resulting
value once the async function has terminated.

* builtins/AsyncFunctionPrototype.js:
(globalPrivate.asyncFunctionResume):
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
* bytecompiler/BytecodeGenerator.h:
(JSC::BytecodeGenerator::promiseCapabilityRegister):
* bytecompiler/NodesCodegen.cpp:
(JSC::FunctionNode::emitBytecode):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208726 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/asyncFunctionTests.yaml
JSTests/stress/async-await-long-loop.js [new file with mode: 0644]
JSTests/stress/async-await-throw-loop.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/builtins/AsyncFunctionPrototype.js
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h
Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp

index 2a4aaf3..b9ff09b 100644 (file)
@@ -1,3 +1,19 @@
+2016-11-14  Caitlin Potter  <caitp@igalia.com>
+
+        [JSC] do not reference AwaitExpression Promises in async function Promise chain
+        https://bugs.webkit.org/show_bug.cgi?id=164753
+
+        Reviewed by Yusuke Suzuki.
+
+        * asyncFunctionTests.yaml:
+        * stress/async-await-long-loop.js: Added.
+        (shouldBe):
+        (async.longLoop):
+        * stress/async-await-throw-loop.js: Added.
+        (shouldBe):
+        (async.thrower):
+        (async.throwLoop):
+
 2016-11-14  Keith Miller  <keith_miller@apple.com>
 
         Add Wasm select
index 1e38bfb..5076dd5 100644 (file)
   cmd: runDefault
 - path: stress/async-await-reserved-word.js
   cmd: runDefault
+- path: stress/async-await-long-loop.js
+  cmd: runNoCJIT "--gcMaxHeapSize=2000000"
+- path: stress/async-await-throw-loop.js
+  cmd: runNoCJIT "--gcMaxHeapSize=2000000"
 
 # FTLJIT
 - path: stress/async-function-create-optimized.js
diff --git a/JSTests/stress/async-await-long-loop.js b/JSTests/stress/async-await-long-loop.js
new file mode 100644 (file)
index 0000000..bbd4ae0
--- /dev/null
@@ -0,0 +1,28 @@
+// Copyright (C) Copyright 2016 the v8 project authors. All rights reserved.
+
+// This test requires ENABLE_ES2017_ASYNCFUNCTION_SYNTAX to be enabled at build time.
+//@ skip
+// TODO: @runNoCJIT("--gcMaxHeapSize=2000000")
+
+
+function shouldBe(expected, actual, msg = "") {
+    if (msg)
+        msg = " for " + msg;
+    if (actual !== expected)
+        throw new Error("bad value" + msg + ": " + actual + ". Expected " + expected);
+}
+
+
+let out;
+
+async function longLoop() {
+    for (let i = 0; i < 10000; i++)
+        await undefined;
+    out = 1;
+}
+
+longLoop();
+
+drainMicrotasks();
+
+shouldBe(out, 1);
diff --git a/JSTests/stress/async-await-throw-loop.js b/JSTests/stress/async-await-throw-loop.js
new file mode 100644 (file)
index 0000000..564ba61
--- /dev/null
@@ -0,0 +1,34 @@
+// Copyright (C) Copyright 2016 the v8 project authors. All rights reserved.
+
+// This test requires ENABLE_ES2017_ASYNCFUNCTION_SYNTAX to be enabled at build time.
+//@ skip
+// TODO: @runNoCJIT("--gcMaxHeapSize=2000000")
+
+function shouldBe(expected, actual, msg = "") {
+    if (msg)
+        msg = " for " + msg;
+    if (actual !== expected)
+        throw new Error("bad value" + msg + ": " + actual + ". Expected " + expected);
+}
+
+
+let out;
+
+async function thrower() { throw undefined; }
+
+async function throwLoop() {
+    for (let i = 0; i < 8000; i++) {
+        try {
+            await thrower();
+        } catch (error) {
+            void 0;
+        }
+    }
+    out = 2;
+}
+
+throwLoop();
+
+drainMicrotasks();
+
+shouldBe(out, 2);
index 1996f0f..1c50290 100644 (file)
@@ -1,3 +1,31 @@
+2016-11-14  Caitlin Potter  <caitp@igalia.com>
+
+        [JSC] do not reference AwaitExpression Promises in async function Promise chain
+        https://bugs.webkit.org/show_bug.cgi?id=164753
+
+        Reviewed by Yusuke Suzuki.
+
+        Previously, long-running async functions which contained many AwaitExpressions
+        would allocate and retain references to intermediate Promise objects for each `await`,
+        resulting in a memory leak.
+
+        To mitigate this leak, a reference to the original Promise (and its resolve and reject
+        functions) associated with the async function are kept, and passed to each call to
+        @asyncFunctionResume, while intermediate Promises are discarded. This is done by adding
+        a new Register to the BytecodeGenerator to hold the PromiseCapability object associated
+        with an async function wrapper. The capability is used to reject the Promise if an
+        exception is thrown during parameter initialization, and is used to store the resulting
+        value once the async function has terminated.
+
+        * builtins/AsyncFunctionPrototype.js:
+        (globalPrivate.asyncFunctionResume):
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        * bytecompiler/BytecodeGenerator.h:
+        (JSC::BytecodeGenerator::promiseCapabilityRegister):
+        * bytecompiler/NodesCodegen.cpp:
+        (JSC::FunctionNode::emitBytecode):
+
 2016-11-14  Joseph Pecoraro  <pecoraro@apple.com>
 
         Web Inspector: Worker debugging should pause all targets and view call frames in all targets
index ea90ef4..8d40367 100644 (file)
@@ -24,7 +24,7 @@
  */
 
 @globalPrivate
-function asyncFunctionResume(generator, sentValue, resumeMode)
+function asyncFunctionResume(generator, promiseCapability, sentValue, resumeMode)
 {
     "use strict";
     let state = generator.@generatorState;
@@ -38,14 +38,18 @@ function asyncFunctionResume(generator, sentValue, resumeMode)
         value = generator.@generatorNext.@call(generator.@generatorThis, generator, state, sentValue, resumeMode, generator.@generatorFrame);
         if (generator.@generatorState === @GeneratorStateExecuting) {
             generator.@generatorState = @GeneratorStateCompleted;
-            return @Promise.@resolve(value);
+            promiseCapability.@resolve(value);
+            return promiseCapability.@promise;
         }
     } catch (error) {
         generator.@generatorState = @GeneratorStateCompleted;
-        return @Promise.@reject(error);
+        promiseCapability.@reject(error);
+        return promiseCapability.@promise;
     }
 
-    return @Promise.@resolve(value).@then(
-        function(value) { return @asyncFunctionResume(generator, value, @GeneratorResumeModeNormal); },
-        function(error) { return @asyncFunctionResume(generator, error, @GeneratorResumeModeThrow); });
+    @Promise.@resolve(value).@then(
+        function(value) { @asyncFunctionResume(generator, promiseCapability, value, @GeneratorResumeModeNormal); },
+        function(error) { @asyncFunctionResume(generator, promiseCapability, error, @GeneratorResumeModeThrow); });
+
+    return promiseCapability.@promise;
 }
index 69b2539..7f0e313 100644 (file)
@@ -562,6 +562,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
         ASSERT(!isConstructor());
         ASSERT(constructorKind() == ConstructorKind::None);
         m_generatorRegister = addVar();
+        m_promiseCapabilityRegister = addVar();
 
         if (parseMode != SourceParseMode::AsyncArrowFunctionMode) {
             // FIXME: Emit to_this only when AsyncFunctionBody uses it.
@@ -574,6 +575,23 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
         }
 
         emitNewObject(m_generatorRegister);
+
+        // let promiseCapability be @newPromiseCapability(@Promise)
+        auto varNewPromiseCapability = variable(propertyNames().builtinNames().newPromiseCapabilityPrivateName());
+        RefPtr<RegisterID> scope = newTemporary();
+        moveToDestinationIfNeeded(scope.get(), emitResolveScope(scope.get(), varNewPromiseCapability));
+        RefPtr<RegisterID> newPromiseCapability = emitGetFromScope(newTemporary(), scope.get(), varNewPromiseCapability, ThrowIfNotFound);
+
+        CallArguments args(*this, nullptr, 1);
+        emitLoad(args.thisRegister(), jsUndefined());
+
+        auto varPromiseConstructor = variable(propertyNames().builtinNames().PromisePrivateName());
+        moveToDestinationIfNeeded(scope.get(), emitResolveScope(scope.get(), varPromiseConstructor));
+        emitGetFromScope(args.argumentRegister(0), scope.get(), varPromiseConstructor, ThrowIfNotFound);
+
+        // JSTextPosition(int _line, int _offset, int _lineStartOffset)
+        JSTextPosition divot(m_scopeNode->firstLine(), m_scopeNode->startOffset(), m_scopeNode->lineStartOffset());
+        emitCall(promiseCapabilityRegister(), newPromiseCapability.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
         break;
     }
 
@@ -674,21 +692,17 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
         RefPtr<Label> catchHere = emitLabel(newLabel().get());
         popTryAndEmitCatch(tryFormalParametersData, exception.get(), thrownValue.get(), catchHere.get(), HandlerType::Catch);
 
-        // return @Promise.@reject(thrownValue);
-        Variable promiseVar = variable(m_vm->propertyNames->builtinNames().PromisePrivateName());
-        RefPtr<RegisterID> scope = emitResolveScope(newTemporary(), promiseVar);
-        RefPtr<RegisterID> promiseConstructor = emitGetFromScope(newTemporary(), scope.get(), promiseVar, ResolveMode::ThrowIfNotFound);
-        RefPtr<RegisterID> promiseReject = emitGetById(newTemporary(), promiseConstructor.get(), m_vm->propertyNames->builtinNames().rejectPrivateName());
+        // return promiseCapability.@reject(thrownValue)
+        RefPtr<RegisterID> reject = emitGetById(newTemporary(), promiseCapabilityRegister(), m_vm->propertyNames->builtinNames().rejectPrivateName());
 
         CallArguments args(*this, nullptr, 1);
-
-        emitMove(args.thisRegister(), promiseConstructor.get());
+        emitLoad(args.thisRegister(), jsUndefined());
         emitMove(args.argumentRegister(0), thrownValue.get());
 
         JSTextPosition divot(functionNode->firstLine(), functionNode->startOffset(), functionNode->lineStartOffset());
 
-        RefPtr<RegisterID> result = emitCall(newTemporary(), promiseReject.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
-        emitReturn(result.get());
+        RefPtr<RegisterID> result = emitCall(newTemporary(), reject.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
+        emitReturn(emitGetById(newTemporary(), promiseCapabilityRegister(), m_vm->propertyNames->builtinNames().promisePrivateName()));
 
         emitLabel(didNotThrow.get());
     }
index 721b396..437a2ed 100644 (file)
@@ -315,6 +315,8 @@ namespace JSC {
 
         RegisterID* generatorRegister() { return m_generatorRegister; }
 
+        RegisterID* promiseCapabilityRegister() { return m_promiseCapabilityRegister; }
+
         // Returns the next available temporary register. Registers returned by
         // newTemporary require a modified form of reference counting: any
         // register with a refcount of 0 is considered "available", meaning that
@@ -931,6 +933,7 @@ namespace JSC {
         RegisterID* m_isDerivedConstuctor { nullptr };
         RegisterID* m_linkTimeConstantRegisters[LinkTimeConstantCount];
         RegisterID* m_arrowFunctionContextLexicalEnvironmentRegister { nullptr };
+        RegisterID* m_promiseCapabilityRegister { nullptr };
 
         SegmentedVector<RegisterID*, 16> m_localRegistersForCalleeSaveRegisters;
         SegmentedVector<RegisterID, 32> m_constantPoolRegisters;
index d3595cf..edb65d7 100644 (file)
@@ -3466,26 +3466,24 @@ void FunctionNode::emitBytecode(BytecodeGenerator& generator, RegisterID*)
         ASSERT(startOffset() >= lineStartOffset());
         generator.emitDebugHook(WillLeaveCallFrame, lastLine(), startOffset(), lineStartOffset());
 
-        // load @asyncFunctionResume, and call.
-        RefPtr<RegisterID> startAsyncFunction = generator.newTemporary();
+        // load and call @asyncFunctionResume
         auto var = generator.variable(generator.propertyNames().builtinNames().asyncFunctionResumePrivateName());
-
         RefPtr<RegisterID> scope = generator.newTemporary();
         generator.moveToDestinationIfNeeded(scope.get(), generator.emitResolveScope(scope.get(), var));
-        generator.emitGetFromScope(startAsyncFunction.get(), scope.get(), var, ThrowIfNotFound);
+        RefPtr<RegisterID> asyncFunctionResume = generator.emitGetFromScope(generator.newTemporary(), scope.get(), var, ThrowIfNotFound);
 
-        // return @asyncFunctionResume.@call(this, @generator, @undefined, GeneratorResumeMode::NormalMode)
-        CallArguments args(generator, nullptr, 3);
+        CallArguments args(generator, nullptr, 4);
         unsigned argumentCount = 0;
         generator.emitLoad(args.thisRegister(), jsUndefined());
         generator.emitMove(args.argumentRegister(argumentCount++), generator.generatorRegister());
+        generator.emitMove(args.argumentRegister(argumentCount++), generator.promiseCapabilityRegister());
         generator.emitLoad(args.argumentRegister(argumentCount++), jsUndefined());
         generator.emitLoad(args.argumentRegister(argumentCount++), jsNumber(static_cast<int32_t>(JSGeneratorFunction::GeneratorResumeMode::NormalMode)));
         // JSTextPosition(int _line, int _offset, int _lineStartOffset)
         JSTextPosition divot(firstLine(), startOffset(), lineStartOffset());
 
         RefPtr<RegisterID> result = generator.newTemporary();
-        generator.emitCallInTailPosition(result.get(), startAsyncFunction.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
+        generator.emitCallInTailPosition(result.get(), asyncFunctionResume.get(), NoExpectedFunction, args, divot, divot, divot, DebuggableCall::No);
         generator.emitReturn(result.get());
         break;
     }