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

Changing PromiseExecutor to use a Rust VecDeque #165

Merged
merged 1 commit into from
Mar 18, 2018

Conversation

Pauan
Copy link
Contributor

@Pauan Pauan commented Mar 17, 2018

The behavior should be identical, but this dramatically improves the performance. In my animation test demo it went from ~10 FPS to ~60 FPS!

There are two reasons for this:

  1. It maintains a VecDeque in Rust, rather than using the browser's event loop queue. This means much less interop between Rust -> JS -> Rust, which means less overhead.

  2. It avoids creating garbage as much as possible.

    The old implementation created 3 functions and 2 Promises per event tick per Task. So if there were 100 Tasks that means it created 300 functions + 200 Promises on every event tick.

    The new implementation creates 1 Promise per event tick. So if there are 100 Tasks that means it creates 1 Promise on every event tick.

    That means multiple orders of magnitude less garbage.

It seems that avoiding garbage is much more important for performance than using VecDeque, but either way it's a huge performance benefit.

After these changes, the biggest performance bottlenecks seem to be from the to_js and to_js_string functions.

@Pauan
Copy link
Contributor Author

Pauan commented Mar 17, 2018

Now that I got this done I can work on getting Discard landed.

@koute
Copy link
Owner

koute commented Mar 18, 2018

Thanks for the PR!

Hmm... this is certainly a creative way to implement a promise queue. Since this functionality is unstable anyway I'll merge it in as-is and we can continue fleshing it out. (We really need some tests and benchmarks.)

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