Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add error stack to .later #250

Merged
merged 3 commits into from
Jan 20, 2018
Merged

add error stack to .later #250

merged 3 commits into from
Jan 20, 2018

Conversation

bekzod
Copy link
Contributor

@bekzod bekzod commented May 27, 2017 •
edited
Loading

  • adds error stack recording to .later as in .schedule .scheduleOnce...
  • avoids creating extra closure that wraps .later callback
  • from benchmarks this does not bring any performance improvements (there is 5% drop in Later & Cancel - function, target case benchmark)

any thoughts ? if you like the idea will squash and cleanup

@bekzod bekzod changed the title add stack to settimeout add stack to settimeout (wip) May 27, 2017
@bekzod bekzod changed the title add stack to settimeout (wip) add stack to later (wip) May 27, 2017
@bekzod bekzod force-pushed the new-set-timeout branch from ba4d16f to 85182a2 Compare May 27, 2017 20:16
@bekzod
Copy link
Contributor Author

bekzod commented May 27, 2017

as mentioned here emberjs/ember.js#15215

@bekzod bekzod force-pushed the new-set-timeout branch 3 times, most recently from 3e59a17 to ec74625 Compare May 29, 2017 07:58
@bekzod bekzod changed the title add stack to later (wip) add error stack to .later (wip) May 30, 2017
@stefanpenner
Copy link
Collaborator

awesome, i like this direction!

@bekzod bekzod force-pushed the new-set-timeout branch from ec74625 to c8aeef2 Compare May 30, 2017 19:54
@bekzod bekzod force-pushed the new-set-timeout branch 3 times, most recently from 85a6281 to 4d4cd37 Compare June 12, 2017 20:51
@bekzod bekzod changed the title add error stack to .later (wip) add error stack to .later Jun 12, 2017
bekzod
bekzod commented Jun 12, 2017
View reviewed changes
lib/index.ts Outdated
let method = timers[i + 3];
let args = timers[i + 4];
let stack = this.DEBUG ? new Error() : undefined;
this.currentInstance.schedule(defaultQueue, target, method, args, false, stack);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not using _ensureInstance() here because we are already in run loop

@bekzod
Copy link
Contributor Author

bekzod commented Jun 12, 2017 •
edited
Loading

only brittle part is: I am identifying timerId type by its value type, if it is integer=> debounce/throttle timer if string=> later timer, but planning to make them both use integer and use bitwise magic to identify timerId type in next PR

@bekzod bekzod force-pushed the new-set-timeout branch 2 times, most recently from 3f60385 to 2f73044 Compare June 13, 2017 07:09
@bekzod
Copy link
Contributor Author

bekzod commented Jun 19, 2017

this looks good, no ? :)

@bekzod
Copy link
Contributor Author

bekzod commented Jul 11, 2017 •
edited
Loading

this is good to go if something :P

@rwjblue
Copy link
Collaborator

rwjblue commented Sep 21, 2017

I resolved the conflicts from recent changes.

@stefanpenner - Mind taking another look?

@bekzod
Copy link
Contributor Author

bekzod commented Nov 21, 2017

@stefanpenner can this be merged :) this might help to further refactor

@bekzod bekzod force-pushed the new-set-timeout branch 5 times, most recently from 06777b3 to af8f9ad Compare December 3, 2017 19:11
@bekzod
Copy link
Contributor Author

bekzod commented Dec 3, 2017

lets merge this 📣 😛

@bekzod
Copy link
Contributor Author

bekzod commented Dec 8, 2017

@rwjblue maybe we should merge this :)

@bekzod
Copy link
Contributor Author

bekzod commented Jan 19, 2018

@stefanpenner thoughts ?

rwjblue
rwjblue requested changes Jan 20, 2018
View reviewed changes
Copy link
Collaborator

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased this to get latest updates to build and linting, which pointed out a type issue that I also pushed a commit fixing.

I also pointed out an issue inline around where the debug stack is being captured. I'll try to take a stab at fixing that if I have a chance. Otherwise this looks great...

lib/index.ts Outdated
let target = timers[i + 2];
let method = timers[i + 3];
let args = timers[i + 4];
let stack = this.DEBUG ? new Error() : undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being done in the wrong place. _scheduleExpiredTimers is after the real setTimeout has ran and we are scheduling the jobs into the actions queue. That will mean the debug stack we are grabbing here is now the stack from where run.later(...) was called.

This needs to be moved into _setTimeout where we are doing this._timers.splice(...) / this._timers.push so that the captured debug stack is the "right" place.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, pushed a fix for this issue to the branch along with a test...

@rwjblue
Move stack capture into _setTimeout.
c50165b 
Add tests ensuring stack capture works properly.
@rwjblue
Copy link
Collaborator

rwjblue commented Jan 20, 2018

I also pointed out an issue inline around where the debug stack is being captured. I'll try to take a stab at fixing that if I have a chance.

Fixed this, and added a test confirming things work nicely.

@rwjblue rwjblue merged commit 61cac06 into BackburnerJS:master Jan 20, 2018
@bekzod bekzod deleted the new-set-timeout branch January 21, 2018 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue approved these changes

@stefanpenner stefanpenner Awaiting requested review from stefanpenner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bekzod @stefanpenner @rwjblue