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

feat(util): use native Map if available #966

Merged
merged 4 commits into from
Nov 1, 2022

Conversation

JonasBa
Copy link
Contributor

@JonasBa JonasBa commented Oct 20, 2022

Related to the discussion in #965

This PR is lacking tests, if the general direction is ok and validated, I will add the relevant tests.

From initial benchmarks

> node src/core/util.benchmark.js
cpu: Apple M1 Pro
runtime: node v18.3.0 (arm64-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
• n=8
------------------------------------------------- -----------------------------
Native      92.59 ns/iter  (88.37 ns … 521.94 ns)  94.09 ns  116.1 ns 131.65 ns
Polyfill   140.06 ns/iter (137.22 ns … 171.45 ns) 139.03 ns 164.76 ns 170.73 ns

summary for n=8
  Native
   1.51x faster than Polyfill

• n=32
------------------------------------------------- -----------------------------
Native      298.8 ns/iter    (292 ns … 649.44 ns) 294.09 ns 630.26 ns 649.44 ns
Polyfill     1.17 µs/iter     (1.15 µs … 1.19 µs)   1.17 µs   1.19 µs   1.19 µs

summary for n=32
  Native
   3.9x faster than Polyfill

• n=64
------------------------------------------------- -----------------------------
Native     558.55 ns/iter (554.65 ns … 581.92 ns)  558.3 ns 580.21 ns 581.92 ns
Polyfill      2.4 µs/iter     (2.38 µs … 2.43 µs)   2.41 µs   2.43 µs   2.43 µs

summary for n=64
  Native
   4.3x faster than Polyfill

• n=128
------------------------------------------------- -----------------------------
Native       1.09 µs/iter     (1.08 µs … 1.13 µs)   1.09 µs   1.13 µs   1.13 µs
Polyfill      4.8 µs/iter     (4.76 µs … 4.84 µs)   4.81 µs   4.84 µs   4.84 µs

summary for n=128
  Native
   4.41x faster than Polyfill

• n=1024
------------------------------------------------- -----------------------------
Native       8.47 µs/iter     (8.46 µs … 8.57 µs)   8.47 µs   8.57 µs   8.57 µs
Polyfill    43.66 µs/iter  (42.83 µs … 236.92 µs)  43.54 µs  46.92 µs  48.67 µs

summary for n=1024
  Native
   5.15x faster than Polyfill

• n=8192
------------------------------------------------- -----------------------------
Native      68.23 µs/iter    (67.5 µs … 93.42 µs)  68.25 µs  72.46 µs  73.46 µs
Polyfill   771.05 µs/iter   (752.29 µs … 1.06 ms) 772.42 µs 871.79 µs 897.46 µs

summary for n=8192
  Native
   11.3x faster than Polyfill

• n=200000
------------------------------------------------- -----------------------------
Native       1.66 ms/iter     (1.64 ms … 1.71 ms)   1.66 ms    1.7 ms    1.7 ms
Polyfill    28.46 ms/iter    (28.15 ms … 29.6 ms)  28.51 ms   29.6 ms   29.6 ms

summary for n=200000
  Native
   17.17x faster than Polyfill

src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
@JonasBa JonasBa force-pushed the jb/core/hashmap-es6 branch from 136f708 to f3f22b6 Compare October 25, 2022 14:06
src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
src/core/util.ts Show resolved Hide resolved
src/core/util.ts Outdated Show resolved Hide resolved
@plainheart plainheart linked an issue Oct 26, 2022 that may be closed by this pull request
Copy link
Collaborator

@plainheart plainheart left a comment

Choose a reason for hiding this comment

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

The others LGTM

src/core/util.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@plainheart plainheart left a comment

Choose a reason for hiding this comment

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

Thank you @JonasBa for the nice work!
@pissang Any other thoughts? If none, we can merge it.

@JonasBa
Copy link
Contributor Author

JonasBa commented Oct 31, 2022

Appreciate the thorough review and helpful comments @plainheart @pissang 🙏

@plainheart plainheart merged commit 20821c1 into ecomfe:master Nov 1, 2022
plainheart added a commit to apache/echarts that referenced this pull request Nov 2, 2022
@plainheart plainheart changed the title feat(polyfill): use native map if available feat(util): use native Map if available Nov 28, 2022
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.

HashMap.each performance
3 participants