Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1985293002:
Promises: Make debug calls only when debugging (Closed)
DescriptionPromises: Make debug calls only when debugging
Previously, certain calls to DebugPushPromise and DebugPopPromise
happened always, without any check to see if we were in a debugging
environment. This patch adds a conditional check before making these
debug calls to make sure they aren't called when not needed.
Before the patch, running --prof over the bluebird benchmarks,
brings up these unprotected debug calls --
ticks cpp total name
16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*)
7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*)
This patch removes the above calls and provides a 4% improvement (with
a 2% variance over 10 runs) in the bluebird benchmark.
Committed: https://crrev.com/9eb320ad10a3040d6604e751551095f6681a45b3
Cr-Commit-Position: refs/heads/master@{#36451}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Use local var #Messages
Total messages: 30 (15 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/1985293002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985293002/1
Description was changed from ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) ========== to ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) ==========
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.
Description was changed from ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) ========== to ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch shaves of 30ms on the bluebird benchmark. ==========
Description was changed from ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch shaves of 30ms on the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and shaves of 30ms on the bluebird benchmark. ==========
More details about the performance analysis would be good for the commit message, but this is a nice fix regardless of the perf impact. https://codereview.chromium.org/1985293002/diff/1/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1985293002/diff/1/src/js/promise.js#newcode88 src/js/promise.js:88: if (DEBUG_IS_ACTIVE) %DebugPushPromise(promise, Promise); Other functions in this file store DEBUG_IS_ACTIVE in a local variable in order to do just one read. Could you do that here, since you have multiple reads in the same function?
Description was changed from ========== Promises: Make debug calls only when debugging Calls to DebugPushPromise and DebugPopPromise are now protected with a conditional. Profiling information -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and shaves of 30ms on the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ==========
Description was changed from ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ==========
PTAL https://codereview.chromium.org/1985293002/diff/1/src/js/promise.js File src/js/promise.js (right): https://codereview.chromium.org/1985293002/diff/1/src/js/promise.js#newcode88 src/js/promise.js:88: if (DEBUG_IS_ACTIVE) %DebugPushPromise(promise, Promise); On 2016/05/18 00:34:46, Dan Ehrenberg wrote: > Other functions in this file store DEBUG_IS_ACTIVE in a local variable in order > to do just one read. Could you do that here, since you have multiple reads in > the same function? I've used a local var called "debug_is_active" which is what some other functions use. Let me know if you'd prefer a more descriptive name.
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/patch-status/1985293002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985293002/20001
lgtm Sounds great. It'd be nice to have one last edit of the commit message to clarify that the --prof runs are *before* the patch, not after.
Description was changed from ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ==========
Description was changed from ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/05/18 01:08:57, Dan Ehrenberg wrote: > lgtm > > Sounds great. It'd be nice to have one last edit of the commit message to > clarify that the --prof runs are *before* the patch, not after. 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/1985293002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1985293002/20001
Message was sent while issue was closed.
Description was changed from ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. ========== to ========== Promises: Make debug calls only when debugging Previously, certain calls to DebugPushPromise and DebugPopPromise happened always, without any check to see if we were in a debugging environment. This patch adds a conditional check before making these debug calls to make sure they aren't called when not needed. Before the patch, running --prof over the bluebird benchmarks, brings up these unprotected debug calls -- ticks cpp total name 16 6.7% 2.0% v8::internal::Runtime_DebugPushPromise(int, v8::internal::Object**, v8::internal::Isolate*) 7 2.9% 0.9% v8::internal::Runtime_DebugPopPromise(int, v8::internal::Object**, v8::internal::Isolate*) This patch removes the above calls and provides a 4% improvement (with a 2% variance over 10 runs) in the bluebird benchmark. Committed: https://crrev.com/9eb320ad10a3040d6604e751551095f6681a45b3 Cr-Commit-Position: refs/heads/master@{#36451} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9eb320ad10a3040d6604e751551095f6681a45b3 Cr-Commit-Position: refs/heads/master@{#36451}
Message was sent while issue was closed.
On 2016/05/23 20:13:18, commit-bot: I haz the power wrote: > Patchset 2 (id:??) landed as > https://crrev.com/9eb320ad10a3040d6604e751551095f6681a45b3 > Cr-Commit-Position: refs/heads/master@{#36451} I'm surprised this does not break any tests, specifically cctest/PromiseRejectCallback. In order for the reject callback to work, we need to push and pop promises regardless of whether debugging is enabled, because that feature is unrelated to the debugger.
Message was sent while issue was closed.
On 2016/05/25 15:56:38, Yang wrote: > On 2016/05/23 20:13:18, commit-bot: I haz the power wrote: > > Patchset 2 (id:??) landed as > > https://crrev.com/9eb320ad10a3040d6604e751551095f6681a45b3 > > Cr-Commit-Position: refs/heads/master@{#36451} > > I'm surprised this does not break any tests, specifically > cctest/PromiseRejectCallback. In order for the reject callback to work, we need > to push and pop promises regardless of whether debugging is enabled, because > that feature is unrelated to the debugger. Never mind. I forgot myself that the implementation of that reject callback no longer relies on the pushed stack of promises, which is now only used for debugging. |
