Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2334323006:
Fix async/await memory leak (Closed)
|
Created:
4 years, 3 months ago by Dan Ehrenberg Modified:
4 years, 3 months ago CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionFix 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}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Also do for catch, and fix another test #
Total comments: 6
Patch Set 3 : Rebase on master, and changes for review #
Total comments: 4
Patch Set 4 : Changes based on review #Patch Set 5 : fix typo #
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
littledan@chromium.org changed reviewers: + adamk@chromium.org, caitp@igalia.com
https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.js File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.... src/js/harmony-async-await.js:72: // The resulting Promise is a throwaway, so it doesn't matter what it don't we have this same problem in the rejection case?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/24198)
https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.js File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.... src/js/harmony-async-await.js:72: // The resulting Promise is a throwaway, so it doesn't matter what it On 2016/09/15 at 20:18:40, caitp wrote: > don't we have this same problem in the rejection case? I don't think so; we don't have the whole chaining thing going on in the rejection case.
https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.js File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.... src/js/harmony-async-await.js:72: // The resulting Promise is a throwaway, so it doesn't matter what it On 2016/09/15 20:24:21, Dan Ehrenberg wrote: > On 2016/09/15 at 20:18:40, caitp wrote: > > don't we have this same problem in the rejection case? > > I don't think so; we don't have the whole chaining thing going on in the > rejection case. Either way, I think it would still be good to add a similar test for ``` async function longLoop2() { for (let i = 0; i < 100000; i++) { try { await thrower(); } catch (e) { } } out = 1; } ``` although it might take too long.
https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.js File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2334323006/diff/1/src/js/harmony-async-await.... src/js/harmony-async-await.js:72: // The resulting Promise is a throwaway, so it doesn't matter what it On 2016/09/15 at 20:32:23, caitp wrote: > On 2016/09/15 20:24:21, Dan Ehrenberg wrote: > > On 2016/09/15 at 20:18:40, caitp wrote: > > > don't we have this same problem in the rejection case? > > > > I don't think so; we don't have the whole chaining thing going on in the > > rejection case. > > Either way, I think it would still be good to add a similar test for > > ``` > async function longLoop2() { > for (let i = 0; i < 100000; i++) { > try { > await thrower(); > } catch (e) { > } > } > out = 1; > } > ``` > > although it might take too long. Oh, you're actually right. It'd similarly take keeping the whole chain alive to figure out the rejection that things should resolve to. I reproduced this in a test, and made a fix in the code, explaining in a comment.
https://codereview.chromium.org/2334323006/diff/20001/src/js/harmony-async-aw... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2334323006/diff/20001/src/js/harmony-async-aw... src/js/harmony-async-await.js:70: var onFulfilled = (sentValue) => { Nit: no need for parens while you're changing this https://codereview.chromium.org/2334323006/diff/20001/src/js/harmony-async-aw... src/js/harmony-async-await.js:78: var onRejected = (sentError) => { ditto https://codereview.chromium.org/2334323006/diff/20001/test/mjsunit/harmony/de... File test/mjsunit/harmony/debug-async-function-async-task-event.js (left): https://codereview.chromium.org/2334323006/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/debug-async-function-async-task-event.js:12: "enqueue #1", Can you mention that you're changing this in your CL description? I assume the change is good...
Description was changed from ========== 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. BUG=v8:5390 ========== to ========== 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 ==========
Rebased without a dependency https://codereview.chromium.org/2334323006/diff/20001/src/js/harmony-async-aw... File src/js/harmony-async-await.js (right): https://codereview.chromium.org/2334323006/diff/20001/src/js/harmony-async-aw... src/js/harmony-async-await.js:70: var onFulfilled = (sentValue) => { On 2016/09/15 at 22:43:20, adamk wrote: > Nit: no need for parens while you're changing this Done https://codereview.chromium.org/2334323006/diff/20001/src/js/harmony-async-aw... src/js/harmony-async-await.js:78: var onRejected = (sentError) => { On 2016/09/15 at 22:43:20, adamk wrote: > ditto Done https://codereview.chromium.org/2334323006/diff/20001/test/mjsunit/harmony/de... File test/mjsunit/harmony/debug-async-function-async-task-event.js (left): https://codereview.chromium.org/2334323006/diff/20001/test/mjsunit/harmony/de... test/mjsunit/harmony/debug-async-function-async-task-event.js:12: "enqueue #1", On 2016/09/15 at 22:43:20, adamk wrote: > Can you mention that you're changing this in your CL description? I assume the change is good... Done
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2334323006/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2334323006/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:4388: // a break location, and the .promise needs to be read earlier so that it The ".promise needs to be read earlier" I don't understand. How is the reading of .promise exposed? https://codereview.chromium.org/2334323006/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:4418: factory()->NewExpressionStatement(promise_assignment, value->position()), This looks weird to me, why does this have a non-kNoSourcePosition? Especially weird since it's the same position used for copying the value below.
https://codereview.chromium.org/2334323006/diff/40001/src/parsing/parser.cc File src/parsing/parser.cc (right): https://codereview.chromium.org/2334323006/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:4388: // a break location, and the .promise needs to be read earlier so that it On 2016/09/16 at 17:24:47, adamk wrote: > The ".promise needs to be read earlier" I don't understand. How is the reading of .promise exposed? This shows up in locations printed in the stack trace. Added a TODO to investigate further. Anyway, we need to read it into a tmp to make it the return value of the DoExpression. https://codereview.chromium.org/2334323006/diff/40001/src/parsing/parser.cc#n... src/parsing/parser.cc:4418: factory()->NewExpressionStatement(promise_assignment, value->position()), On 2016/09/16 at 17:24:47, adamk wrote: > This looks weird to me, why does this have a non-kNoSourcePosition? Especially weird since it's the same position used for copying the value below. Oops, somehow lost the change to make it nopos in a rebase. Fixed.
lgtm
The CQ bit was checked by littledan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux64_avx2_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_mac_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel_ng/builds/9054)
The CQ bit was checked by littledan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from adamk@chromium.org Link to the patchset: https://codereview.chromium.org/2334323006/#ps80001 (title: "fix typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/a0ba18e9634c5e2d439033ab61a77cff54f9af35 Cr-Commit-Position: refs/heads/master@{#39479}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2354473002/ by hablich@chromium.org. The reason for reverting is: newly introduced test async-await-loop times out: https://build.chromium.org/p/client.v8/builders/V8%20Linux%20-%20arm64%20-%20.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
