Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2007803002:
Promises: Lazily create arrays to store resolve, reject callbacks (Closed)
DescriptionPromises: Lazily create arrays to store resolve, reject callbacks
For the common use case of having a single resolve or reject callback,
the callbacks are stored directly. Only when an additional callback is
registered, we create an array to store these callbacks.
There are 3 possible states for the resolve, reject symbols when we add
a new callback --
1) UNDEFINED -- This is the zero state where there is no callback
registered. When we see this state, we directly attach the callbacks to
the symbol.
2) !IS_ARRAY -- There is a single callback directly attached to the
symbols. We need to create a new array to store additional callbacks.
3) IS_ARRAY -- There are multiple callbacks already registered,
therefore we can just push the new callback to the existing array.
Also, this change creates a new symbol for storing the deferred objects.
Previously the deferred objects were stored in the callback arrays, but
since we no longer create arrays for the initial case, we need this new
symbol. The cctest has been updated to account for this new symbol.
This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark.
BUG=v8:5046
Committed: https://crrev.com/1d4fe00287eabfb64d6bba51d685b2e668f179af
Cr-Commit-Position: refs/heads/master@{#36536}
Patch Set 1 #Patch Set 2 : shave yak #Patch Set 3 : Remove length #Patch Set 4 : Fix tests #Patch Set 5 : Update cctest #
Total comments: 24
Patch Set 6 : Address review comments #Patch Set 7 : Refactor PromiseHasUserDefinedRejectHandlerRecursive #Patch Set 8 : minor cleanup #
Messages
Total messages: 35 (18 generated)
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 18% improvement in the bluebird benchmark. ==========
gsathya@chromium.org changed reviewers: + adamk@chromium.org, littledan@chromium.org
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 18% improvement in the bluebird benchmark. ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 18% improvement in the bluebird benchmark. ==========
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 18% improvement in the bluebird benchmark. ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 18% improvement in the bluebird benchmark. ==========
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 18% improvement in the bluebird benchmark. ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. ==========
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. ==========
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007803002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007803002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
The CQ bit was unchecked by gsathya@chromium.org
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/2007803002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007803002/60001
This could use some additional clean up especially in PromiseThen and PromiseHasUserDefinedRejectHandlerRecursive, but sending it out for some early review comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux64_avx2_rel_ng on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng/buil...) v8_linux64_avx2_rel_ng_triggered on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_avx2_rel_ng_trig...)
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. ==========
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/2007803002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007803002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Dry run: None
https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:104: promise, status, value, onResolve, onReject, deferred) { None of the callers pass more than 3 arguments anymore, I think you can just leave off the last three arguments and SET_PRIVATE(..., UNDEFINED) below. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:107: SET_PRIVATE(promise, promiseFulfillReactionsSymbol, onResolve); Maybe add a comment here with a version of the third paragraph of your CL description, explaining the states the last three symbols could be in? https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:128: if (!IS_NULL_OR_UNDEFINED(tasks)) { It should never be null, right? Can this just be IS_UNDEFINED? Same question for other such calls in this file. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:333: case kPending: This whole block seems like it should be encapsulated in a helper function. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:339: } else if (!IS_ARRAY(resolveCallbacks)) { Did you figure out if Bluebird goes down this path? It seems like it still might be worth avoiding creating the deferreds array as previously discussed. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:345: resolveCallbacks.push(GET_PRIVATE(this, promiseFulfillReactionsSymbol)); This extra GET isn't needed, you already had an array in resolveCallbacks. Just choose a different name for that var. so it doesn't get shadowed here. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:357: GET_PRIVATE(this, promiseFulfillReactionsSymbol).push(onResolve); This extra GET isn't needed, you already have an array in resolveCallbacks. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:364: kFulfilled, 1); Looks like a stray change from the previous approach? https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:374: kRejected, 1); Same here. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:488: queue = [queue]; This is a neat trick, I'll have to think about whether I like this more than just factoring the loop body out into its own function.
Thanks for the review! PTAL https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:104: promise, status, value, onResolve, onReject, deferred) { On 2016/05/25 19:18:57, adamk wrote: > None of the callers pass more than 3 arguments anymore, I think you can just > leave off the last three arguments and SET_PRIVATE(..., UNDEFINED) below. Done. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:107: SET_PRIVATE(promise, promiseFulfillReactionsSymbol, onResolve); On 2016/05/25 19:18:57, adamk wrote: > Maybe add a comment here with a version of the third paragraph of your CL > description, explaining the states the last three symbols could be in? Done. Let me know if you can think of a more cohesive explanation https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:128: if (!IS_NULL_OR_UNDEFINED(tasks)) { On 2016/05/25 19:18:57, adamk wrote: > It should never be null, right? Can this just be IS_UNDEFINED? Same question for > other such calls in this file. Done. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:333: case kPending: On 2016/05/25 19:18:57, adamk wrote: > This whole block seems like it should be encapsulated in a helper function. Done. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:339: } else if (!IS_ARRAY(resolveCallbacks)) { On 2016/05/25 19:18:57, adamk wrote: > Did you figure out if Bluebird goes down this path? It seems like it still might > be worth avoiding creating the deferreds array as previously discussed. Bluebird doesn't create arrays at all, they just store the callbacks at an index in the promise object itself like this -- this[deferredOffset] = deferred this[resolveOffset] = onResolve this[rejectOffset] = onReject Removing the deferred array will make the code a little harder to reason about, for example -- 1) For the initial state, we store the deferred object in the deferred symbol 2) For subsequent states where there is more than one callback, we can store the deferred object in the resolve/reject callback arrays (like we've been doing before this patch). When we hit the subsequent state of having more than one callback, we are left with a stale deferred symbol. I could make this a null but I need to add additionally state handling, which makes the code harder to understand and reason about. I feel like this change can be done in a follow up CL. Thoughts? https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:345: resolveCallbacks.push(GET_PRIVATE(this, promiseFulfillReactionsSymbol)); On 2016/05/25 19:18:57, adamk wrote: > This extra GET isn't needed, you already had an array in resolveCallbacks. Just > choose a different name for that var. so it doesn't get shadowed here. Done. Let me know if you have a better name for that var. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:357: GET_PRIVATE(this, promiseFulfillReactionsSymbol).push(onResolve); On 2016/05/25 19:18:57, adamk wrote: > This extra GET isn't needed, you already have an array in resolveCallbacks. Done. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:364: kFulfilled, 1); On 2016/05/25 19:18:57, adamk wrote: > Looks like a stray change from the previous approach? Done. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:374: kRejected, 1); On 2016/05/25 19:18:57, adamk wrote: > Same here. Done. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:488: queue = [queue]; On 2016/05/25 19:18:57, adamk wrote: > This is a neat trick, I'll have to think about whether I like this more than > just factoring the loop body out into its own function. :P Happy to change it, let me know
https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:339: } else if (!IS_ARRAY(resolveCallbacks)) { On 2016/05/25 21:46:35, gsathya wrote: > On 2016/05/25 19:18:57, adamk wrote: > > Did you figure out if Bluebird goes down this path? It seems like it still > might > > be worth avoiding creating the deferreds array as previously discussed. > > Bluebird doesn't create arrays at all, they just store the callbacks at an index > in the promise object itself like this -- > this[deferredOffset] = deferred > this[resolveOffset] = onResolve > this[rejectOffset] = onReject Sorry, when I say "Bluebird" I meant the benchmarks: do they ever call then() more than once on a promise? > Removing the deferred array will make the code a little harder to reason about, > for example -- > 1) For the initial state, we store the deferred object in the deferred symbol > 2) For subsequent states where there is more than one callback, we can store the > deferred object in the resolve/reject callback arrays (like we've been doing > before this patch). > > When we hit the subsequent state of having more than one callback, we are left > with a stale deferred symbol. I could make this a null but I need to add > additionally state handling, which makes the code harder to understand and > reason about. The additional "end" state is definitely the weirdest part of this. One way to make it more readable would be to use constants like we do for [[PromiseState]] for the "zero" and "stale" cases. WDYT? > I feel like this change can be done in a follow up CL. Thoughts? For me this depends on whether the benchmarks we're using do anything to stress the multiple-listener case.
https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:339: } else if (!IS_ARRAY(resolveCallbacks)) { On 2016/05/25 21:57:43, adamk wrote: > On 2016/05/25 21:46:35, gsathya wrote: > > On 2016/05/25 19:18:57, adamk wrote: > > > Did you figure out if Bluebird goes down this path? It seems like it still > > might > > > be worth avoiding creating the deferreds array as previously discussed. > > > > Bluebird doesn't create arrays at all, they just store the callbacks at an > index > > in the promise object itself like this -- > > this[deferredOffset] = deferred > > this[resolveOffset] = onResolve > > this[rejectOffset] = onReject > > > Sorry, when I say "Bluebird" I meant the benchmarks: do they ever call then() > more than once on a promise? > > > Removing the deferred array will make the code a little harder to reason > about, > > for example -- > > 1) For the initial state, we store the deferred object in the deferred symbol > > 2) For subsequent states where there is more than one callback, we can store > the > > deferred object in the resolve/reject callback arrays (like we've been doing > > before this patch). > > > > When we hit the subsequent state of having more than one callback, we are left > > with a stale deferred symbol. I could make this a null but I need to add > > additionally state handling, which makes the code harder to understand and > > reason about. > > The additional "end" state is definitely the weirdest part of this. One way to > make it more readable would be to use constants like we do for [[PromiseState]] > for the "zero" and "stale" cases. WDYT? > > > I feel like this change can be done in a follow up CL. Thoughts? > > For me this depends on whether the benchmarks we're using do anything to stress > the multiple-listener case. As discussed offline, though this path isn't covered by the benchmark, let's land this first and then remove the extra array in a followup. https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:488: queue = [queue]; On 2016/05/25 21:46:35, gsathya wrote: > On 2016/05/25 19:18:57, adamk wrote: > > This is a neat trick, I'll have to think about whether I like this more than > > just factoring the loop body out into its own function. > > :P Happy to change it, let me know Please do change it to a factored-out function and either a loop or just a call to that function as appropriate.
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. BUG=v8:5046 ==========
Thanks for the review! PTAL https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/2007803002/diff/80001/src/js/promise.js#newco... src/js/promise.js:488: queue = [queue]; On 2016/05/25 23:41:35, adamk wrote: > On 2016/05/25 21:46:35, gsathya wrote: > > On 2016/05/25 19:18:57, adamk wrote: > > > This is a neat trick, I'll have to think about whether I like this more than > > > just factoring the loop body out into its own function. > > > > :P Happy to change it, let me know > > Please do change it to a factored-out function and either a loop or just a call > to that function as appropriate. Done.
lgtm
The CQ bit was checked by gsathya@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2007803002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2007803002/140001
Message was sent while issue was closed.
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. BUG=v8:5046 ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. BUG=v8:5046 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. BUG=v8:5046 ========== to ========== Promises: Lazily create arrays to store resolve, reject callbacks For the common use case of having a single resolve or reject callback, the callbacks are stored directly. Only when an additional callback is registered, we create an array to store these callbacks. There are 3 possible states for the resolve, reject symbols when we add a new callback -- 1) UNDEFINED -- This is the zero state where there is no callback registered. When we see this state, we directly attach the callbacks to the symbol. 2) !IS_ARRAY -- There is a single callback directly attached to the symbols. We need to create a new array to store additional callbacks. 3) IS_ARRAY -- There are multiple callbacks already registered, therefore we can just push the new callback to the existing array. Also, this change creates a new symbol for storing the deferred objects. Previously the deferred objects were stored in the callback arrays, but since we no longer create arrays for the initial case, we need this new symbol. The cctest has been updated to account for this new symbol. This patch results in a 19% improvement(over 5 runs) in the bluebird benchmark. BUG=v8:5046 Committed: https://crrev.com/1d4fe00287eabfb64d6bba51d685b2e668f179af Cr-Commit-Position: refs/heads/master@{#36536} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1d4fe00287eabfb64d6bba51d685b2e668f179af Cr-Commit-Position: refs/heads/master@{#36536} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
