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

(fix): performance.now() should be a minimal op to avoid JSON deserialization overhead #8619

Merged
merged 6 commits into from
Dec 7, 2020

Conversation

Soremwar
Copy link
Contributor

@Soremwar Soremwar commented Dec 4, 2020

Closes #8548

@Soremwar
Copy link
Contributor Author

Soremwar commented Dec 5, 2020

A million iterations

Minimal OP

performance.now - Time elapsed: 2335 ms
performance.mark - Time elapsed: 3388 ms

JSON OP

performance.now - Time elapsed: 5906 ms
performance.mark - Time elapsed: 7564 ms

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

@Soremwar looks good!

cli/ops/timers.rs Show resolved Hide resolved
cli/ops/timers.rs Outdated Show resolved Hide resolved
cli/rt/11_timers.js Outdated Show resolved Hide resolved
@Soremwar
Copy link
Contributor Author

Soremwar commented Dec 5, 2020

Removing the extra Uint8array actually made a big difference :O

New times:

Minimal OP v1 (Extra Uint8Array)

performance.now - Time elapsed: 2335 ms
performance.mark - Time elapsed: 3388 ms

Minimal OP v2 (Clean)

performance.now - Time elapsed: 1677 ms
performance.mark - Time elapsed: 2921 ms

JSON OP

performance.now - Time elapsed: 5906 ms
performance.mark - Time elapsed: 7564 ms

@Soremwar
Copy link
Contributor Author

Soremwar commented Dec 5, 2020

Stll quite slow compared to Node, I might look inside to check what magic they are doing to achieve those results, but for all practical purposes this is solved

@bnoordhuis
Copy link
Contributor

@Soremwar Node.js uses V8 fast API calls (nodejs/node#33600). I have a proof-of-concept in denoland/rusty_v8#511 but it's not anywhere near ready.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 6, 2020

Yeah, so this improvement is great for now until we can support fast API.

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

Nice work @Soremwar - LGTM

This is a very encouraging example of why we should support FastAPI. Ultimately it would be awesome if all of our ops could use it.

@ry ry merged commit 43a35b0 into denoland:master Dec 7, 2020
@Soremwar Soremwar deleted the performance branch December 29, 2021 19:21
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.

performance.now() should be a minimal op to avoid JSON deserialization overhead
5 participants