Skip to content

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

Merged
ry merged 6 commits into
denoland:masterfrom
Soremwar:performance
Dec 7, 2020
Merged

(fix): performance.now() should be a minimal op to avoid JSON deserialization overhead#8619
ry merged 6 commits into
denoland:masterfrom
Soremwar:performance

Conversation

@Soremwar

@Soremwar Soremwar commented Dec 4, 2020

Copy link
Copy Markdown
Contributor

Closes #8548

@Soremwar

Soremwar commented Dec 5, 2020

Copy link
Copy Markdown
Contributor Author

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

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@Soremwar looks good!

Comment thread cli/ops/timers.rs
Comment thread cli/ops/timers.rs Outdated
Soremwar
Soremwar commented Dec 5, 2020
View reviewed changes
Comment thread cli/rt/11_timers.js Outdated
@Soremwar

Soremwar commented Dec 5, 2020

Copy link
Copy Markdown
Contributor Author

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

Soremwar commented Dec 5, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

bnoordhuis commented Dec 5, 2020

Copy link
Copy Markdown
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

kitsonk commented Dec 6, 2020

Copy link
Copy Markdown
Contributor

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

@ry ry left a comment

Copy link
Copy Markdown
Member

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