Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

perf(ext/web): optimize performance.measure() #25774

Merged
merged 1 commit into from
Sep 20, 2024

Conversation

esroyo
Copy link
Contributor

@esroyo esroyo commented Sep 20, 2024

This PR optimizes the case when performance.measure() needs to find the startMark by name. It is a simple change on findMostRecent fn to avoiding copying and reversing the complete entries list.

Adds minor missing tests for:

  • clearMarks(), general
  • clearMeasures(), general
  • measure(), case when the startMarks name exists more than once

Benchmarks

main

    CPU | AMD Ryzen 7 PRO 6850U with Radeon Graphics
Runtime | Deno 2.0.0-rc.4 (x86_64-unknown-linux-gnu)

benchmark              time/iter (avg)        iter/s      (min … max)           p75      p99     p995
---------------------- ----------------------------- --------------------- --------------------------
worst case measure()            2.1 ms         486.9 (  1.7 ms …   2.4 ms)   2.2 ms   2.4 ms   2.4 ms

this PR

    CPU | AMD Ryzen 7 PRO 6850U with Radeon Graphics
Runtime | Deno 2.0.0-rc.4 (x86_64-unknown-linux-gnu)

benchmark              time/iter (avg)        iter/s      (min … max)           p75      p99     p995
---------------------- ----------------------------- --------------------- --------------------------
worst case measure()          966.3 µs         1,035 (876.9 µs …   1.1 ms)   1.0 ms   1.1 ms   1.1 ms
Deno.bench("worst case measure()", (b) => {
  performance.mark('start');

  for (let i = 0; i < 1e5; i += 1) {
    performance.mark(crypto.randomUUID());
  }

  b.start();

  performance.measure('total', 'start');

  b.end();

  performance.clearMarks();
  performance.clearMeasures();
});

@CLAassistant
Copy link

CLAassistant commented Sep 20, 2024

CLA assistant check
All committers have signed the CLA.

Optimizes the case when `performance.measure()` needs to find the
startMark by name, by avoiding copying and reversing the complete
entries list.
@esroyo esroyo force-pushed the perf-performance-measure branch from f6a2899 to ca392b4 Compare September 20, 2024 21:22
Copy link
Contributor

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

LGTM, that's a nice performance win!!

Copy link
Member

@nathanwhit nathanwhit left a comment

Choose a reason for hiding this comment

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

LGTM, nice!

@nathanwhit nathanwhit merged commit 88a469e into denoland:main Sep 20, 2024
17 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants