-
Notifications
You must be signed in to change notification settings - Fork 80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal] Async stack traces. #340
[Proposal] Async stack traces. #340
Conversation
47e3944
to
8517330
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this problem! The one thing that strikes me here is that we may lose some of the ordering guarantees that we have today... 🤔
@krisselden / @stefanpenner / @ef4 - Thoughts?
lib/index.ts
Outdated
.catch((err) => { | ||
let onError = getOnError(this.options); | ||
|
||
if (onError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The else
case should throw err
here (or we swallow any/all errors when there is not an Ember.onerror
set).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, nicely spotted! I'll update the PR to throw the error.
lib/index.ts
Outdated
let resolve; | ||
|
||
new Promise((_resolve) => resolve = _resolve) | ||
.then(method.bind(target, ...args)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure if binding here is any better/faster than .then(() => method.apply(target, args))
, might be worth testing...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, your suggestion is much faster: https://jsperf.com/bind-creation-vs-closure-creation/1
I'll add that to the PR.
Edit: Ran the benchmarks again and there seems to be some improvement in performance (against master too!). Nice!
The new benchmarks (I've updated the original post):
Benchmark | master | PR
------------------------------------------------------------------------------------------------------------------------
DEBUG - Schedule & Flush - function ................................................ | 126,951.07 op/s | 139,381.79 op/s
DEBUG - Schedule & Flush - target, function ........................................ | 126,113.43 op/s | 130,448.98 op/s
DEBUG - Schedule & Flush - target, string method name .............................. | 130,435.87 op/s | 128,308.48 op/s
DEBUG - Schedule & Flush - target, string method name, 1 argument .................. | 133,912.92 op/s | 125,822.98 op/s
DEBUG - Schedule & Flush - target, string method name, 2 arguments ................. | 132,708.16 op/s | 122,860.41 op/s
Old benchmarks for posterity:
Benchmark | master | PR
------------------------------------------------------------------------------------------------------------------------
DEBUG - Schedule & Flush - function ................................................ | 126,951.07 op/s | 112,396.69 op/s
DEBUG - Schedule & Flush - target, function ........................................ | 126,113.43 op/s | 118,414.02 op/s
DEBUG - Schedule & Flush - target, string method name .............................. | 130,435.87 op/s | 125,710.38 op/s
DEBUG - Schedule & Flush - target, string method name, 1 argument .................. | 133,912.92 op/s | 128,377.54 op/s
DEBUG - Schedule & Flush - target, string method name, 2 arguments ................. | 132,708.16 op/s | 129,048.85 op/s
Thanks for the review @rwjblue! Can you elaborate on your thinking with the ordering guarantees? I had some suspicions myself since Promise execution isn't sync but the tests passed so I figured it might have been alright. Can you think of any ideas on how to design a test case for it? |
I don't think this will break anything as long as Promise.then is using proper microtask timing. But I don't think all our supported browsers have correct microtask timing when you use Promise.then. That's why backburner itself is using MutationObserver instead when it needs a microtask wait. Does dev tools stitch together async frames across backburner's I also worry that it's risky to change the timing behavior when the debug flag is on vs off. It seems possible that merely turning on DEBUG would make your bug disappear, or make new bugs appear. For example, this change is enough to alter the behavior of a bug like #332. I would feel better if we just confirmed that there's no performance hit and made this the permanent behavior. I would not expect a performance hit as long as these are native microtasks. |
Tests do not currently run with DEBUG on and off (we should fix that though)... |
In general, I agree. I think the issue here would be that I don't think we can change |
This PR introduces async stack traces to Backburner so it's easier to debug the runloop by allowing the developer to step back through the asynchronous stack frame.
Before / After Stack Trace for this demo code
gif of stepping though demo call stack
After some cursory research, it doesn't look like there's an API for custom async stack traces available in Chrome. It was actually requested from what looks like an Ember developer however that was back in 2014 and there hasn't been much activity on it since. This only left the hacky approach of emulating what Chrome thinks is an async action via Promises. I've created a new method on the
Backburner
class called_asyncExecute
that takes a method and returns a new Promise'sresolve
method (not the promise itself!) synchronously. When that returnedresolve
method is called by Backburner (as if it was themethod
itself), the promise chain advances and calls the original method passed to_asyncExecute
. This tricks the Dev Tools into thinking some asynchronous action happened when in fact it's just a simple callback.Thankfully we only need to patch two methods,
schedule
andlater
.later
doesn't entirely need to be patched because Chrome can accurately represent it's async operation as it's asetTimeout
however it also getsschedule
d into the default queue for execution once the timeout completes which is two async hops. With patchinglater
, we reduce this to one async hop for clarity. All of this of course only happens whenBackburner#DEBUG
is true. Finally, there is no breaking API changes.The method is a little hacky and the
Promise.then (async)
in between async stack frames in the sidebar Call Stack (see gif) is a little misleading but I'd consider this a minor setback for an increase to debuggability and productivity. One of my biggest gripes with Ember is how frustrating it is to debug the runloop.I expected a performance hit to Backburner in debug mode because of the promises ontop of the current stack stitching system but to my surprise, there's barely any. I suspect it's the
new Error
call that is causing such a performance hit between non-debug and debug modes. When Firefox and Safari (and Edge?) supports async stack traces, maybe we could remove that call altogether and getDEBUG
performance to a reasonable level?`node bench` on master
`node bench` on this PR
I've stuck up a page on Github that gives you a demo. Be sure to open Dev Tools and stick a breakpoint in
done
.http://adriancooney.ie/backburner.js/dist/demo/
Todo: