Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2018913004:
Promises: Remove additional array for storing deferred objects (Closed)
DescriptionPromises: Remove additional array for storing deferred objects
There are 2 possible states for the deferred symbol --
1) UNDEFINED -- This is the zero state, no deferred object is attached
to this symbol. When we want to add a new deferred we directly
attach it to this symbol.
2) symbol with attached deferred object -- New deferred objects are
not attached to this symbol, but instead they are directly attached
to the resolve, reject callback arrays. At this point, the deferred
symbol's state is stale, and the deferreds should be read from the
reject, resolve callbacks.
BUG=v8:5046
Committed: https://crrev.com/e3bd4a396b44eb490360f14bcbdc1b36482c8558
Cr-Commit-Position: refs/heads/master@{#36623}
Patch Set 1 #Patch Set 2 : Add the actual changes #
Total comments: 1
Patch Set 3 : Add space around + #
Total comments: 1
Patch Set 4 : Merge calls to array.push #Messages
Total messages: 18 (8 generated)
The CQ bit was checked by gsathya@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018913004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018913004/20001
Description was changed from ========== Promises: Remove additional array for storing deferred objects There are 2 possible states for the deferred symbol -- 1) UNDEFINED -- This is the zero state, no deferred object is attached to this symbol. When we want to add a new deferred we directly attach it to this symbol. 2) symbol with attached deferred object -- New deferred objects are not attached to this symbol, but instead they are directly attached to the resolve, reject callback arrays. At this point, the deferred symbol's state is stale, and the deferreds should be read from the reject, resolve callbacks. BUG=v8:5046 ========== to ========== Promises: Remove additional array for storing deferred objects There are 2 possible states for the deferred symbol -- 1) UNDEFINED -- This is the zero state, no deferred object is attached to this symbol. When we want to add a new deferred we directly attach it to this symbol. 2) symbol with attached deferred object -- New deferred objects are not attached to this symbol, but instead they are directly attached to the resolve, reject callback arrays. At this point, the deferred symbol's state is stale, and the deferreds should be read from the reject, resolve callbacks. BUG=v8:5046 ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ah, so the approach is just to keep an extra reference to the deferred, but it doesn't matter since it's already referenced from the arrays? lgtm % nit https://codereview.chromium.org/2018913004/diff/20001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2018913004/diff/20001/src/js/promise.js#newco... src/js/promise.js:178: PromiseHandle(value, tasks[i], tasks[i+1]); Nit: please add spaces around "+".
On 2016/05/27 23:04:51, adamk wrote: > Ah, so the approach is just to keep an extra reference to the deferred, but it > doesn't matter since it's already referenced from the arrays? Yeah, I could assign it UNDEFINED, NULL, or kDONTUSEMEIMSTALESTATE if you think that's better? > lgtm % nit > > https://codereview.chromium.org/2018913004/diff/20001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/2018913004/diff/20001/src/js/promise.js#newco... > src/js/promise.js:178: PromiseHandle(value, tasks[i], tasks[i+1]); > Nit: please add spaces around "+". Done
On 2016/05/27 23:11:11, gsathya wrote: > On 2016/05/27 23:04:51, adamk wrote: > > Ah, so the approach is just to keep an extra reference to the deferred, but it > > doesn't matter since it's already referenced from the arrays? > > Yeah, I could assign it UNDEFINED, NULL, or kDONTUSEMEIMSTALESTATE if you think > that's better? As long as it doesn't leak, I'm ok with leaving it literally stale (since we'll never even look it up again). https://codereview.chromium.org/2018913004/diff/40001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2018913004/diff/40001/src/js/promise.js#newco... src/js/promise.js:210: resolveCallbacks.push(onResolve, deferred); This and the below should be able to be combined with the above pushes, right?
On 2016/05/27 23:24:19, adamk wrote: > On 2016/05/27 23:11:11, gsathya wrote: > > On 2016/05/27 23:04:51, adamk wrote: > > > Ah, so the approach is just to keep an extra reference to the deferred, but > it > > > doesn't matter since it's already referenced from the arrays? > > > > Yeah, I could assign it UNDEFINED, NULL, or kDONTUSEMEIMSTALESTATE if you > think > > that's better? > > As long as it doesn't leak, I'm ok with leaving it literally stale (since we'll > never even look it up again). ACK. I don't see how this leaks, so leaving as such. > https://codereview.chromium.org/2018913004/diff/40001/src/js/promise.js > File src/js/promise.js (right): > > https://codereview.chromium.org/2018913004/diff/40001/src/js/promise.js#newco... > src/js/promise.js:210: resolveCallbacks.push(onResolve, deferred); > This and the below should be able to be combined with the above pushes, right? Done.
The CQ bit was checked by gsathya@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/2018913004/#ps60001 (title: "Merge calls to array.push")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2018913004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2018913004/60001
Message was sent while issue was closed.
Description was changed from ========== Promises: Remove additional array for storing deferred objects There are 2 possible states for the deferred symbol -- 1) UNDEFINED -- This is the zero state, no deferred object is attached to this symbol. When we want to add a new deferred we directly attach it to this symbol. 2) symbol with attached deferred object -- New deferred objects are not attached to this symbol, but instead they are directly attached to the resolve, reject callback arrays. At this point, the deferred symbol's state is stale, and the deferreds should be read from the reject, resolve callbacks. BUG=v8:5046 ========== to ========== Promises: Remove additional array for storing deferred objects There are 2 possible states for the deferred symbol -- 1) UNDEFINED -- This is the zero state, no deferred object is attached to this symbol. When we want to add a new deferred we directly attach it to this symbol. 2) symbol with attached deferred object -- New deferred objects are not attached to this symbol, but instead they are directly attached to the resolve, reject callback arrays. At this point, the deferred symbol's state is stale, and the deferreds should be read from the reject, resolve callbacks. BUG=v8:5046 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Promises: Remove additional array for storing deferred objects There are 2 possible states for the deferred symbol -- 1) UNDEFINED -- This is the zero state, no deferred object is attached to this symbol. When we want to add a new deferred we directly attach it to this symbol. 2) symbol with attached deferred object -- New deferred objects are not attached to this symbol, but instead they are directly attached to the resolve, reject callback arrays. At this point, the deferred symbol's state is stale, and the deferreds should be read from the reject, resolve callbacks. BUG=v8:5046 ========== to ========== Promises: Remove additional array for storing deferred objects There are 2 possible states for the deferred symbol -- 1) UNDEFINED -- This is the zero state, no deferred object is attached to this symbol. When we want to add a new deferred we directly attach it to this symbol. 2) symbol with attached deferred object -- New deferred objects are not attached to this symbol, but instead they are directly attached to the resolve, reject callback arrays. At this point, the deferred symbol's state is stale, and the deferreds should be read from the reject, resolve callbacks. BUG=v8:5046 Committed: https://crrev.com/e3bd4a396b44eb490360f14bcbdc1b36482c8558 Cr-Commit-Position: refs/heads/master@{#36623} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e3bd4a396b44eb490360f14bcbdc1b36482c8558 Cr-Commit-Position: refs/heads/master@{#36623} |
