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.

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

perf: Reuse the same promise instance in the scheduler #2555

Merged
merged 1 commit into from
Apr 25, 2019

Conversation

benlesh
Copy link
Contributor

@benlesh benlesh commented Apr 25, 2019

  • Scheduling an update no longer allocates two new promises, only one via then.
  • The tick method was really returning an empty, resolved promise, so just reusing the same promise (resolved_promise) there as well. It was a little confusing because it looked like it would return a promise to when the flush had completed, but in fact it was returning a promise that would resolve before the flush completed, as it was the promise that was "thenned" for the flush.

@Rich-Harris I tried to follow your coding conventions, hopefully it's adequate.

- Scheduling an update no longer allocates two new promises, only one via `then`.
- The `tick` method was really returning an empty, resolved promise, so just reusing the same promise there as well. It was a little confusing because it _looked_ like it would return a promise to when the flush had completed, but in fact it was returning a promise that would resolve before the flush completed, as it was the promise that was "thenned" for the flush.
@Rich-Harris Rich-Harris merged commit ed7c32d into sveltejs:master Apr 25, 2019
@Rich-Harris
Copy link
Member

d'oh! Kicking myself for not seeing this. Thank you!

Yeah, i've been trying to convince everyone around me to adopt snake_case... hopefully it will catch on one day.

@benlesh
Copy link
Contributor Author

benlesh commented Apr 25, 2019

No problem. I look at a lot of scheduler-type code, and it's tricky, so I decided to take a peek.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants