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

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

Merged
merged 6 commits into from
Dec 7, 2020
Merged

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

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

bartlomieju
bartlomieju reviewed Dec 5, 2020
View reviewed changes
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
Soremwar
Soremwar commented Dec 5, 2020
View reviewed changes
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 •
edited
Loading

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.

ry
ry approved these changes Dec 7, 2020
View reviewed changes
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
Reviewers

@bartlomieju bartlomieju bartlomieju left review comments

@ry ry ry approved these changes

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

Successfully merging this pull request may close these issues.

performance.now() should be a minimal op to avoid JSON deserialization overhead
5 participants
@Soremwar @bnoordhuis @kitsonk @ry @bartlomieju