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

Investigate slow new URL #33

Closed
anonrig opened this issue Dec 15, 2022 · 11 comments
Closed

Investigate slow new URL #33

anonrig opened this issue Dec 15, 2022 · 11 comments
Assignees

Comments

@anonrig
Copy link
Member

anonrig commented Dec 15, 2022

It is come to my attention that recent benchmarks run from folks at deno show that new URL invocation is super slow in node. Referencing: https://twitter.com/deno_land/status/1598394826588028928?s=61&t=qr5eBcWZkP3rwIdIefHWdA

@anonrig
Copy link
Member Author

anonrig commented Dec 15, 2022

My local benchmark result for new URL('http://example.com'):

  • Node
cpu: Apple M1 Max
runtime: node v20.0.0-pre (arm64-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
new URL     678.5 ns/iter  (659.13 ns … 744.8 ns) 681.77 ns  744.8 ns  744.8 ns
  • Bun
cpu: Apple M1 Max
runtime: bun 0.3.0 (arm64-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
new URL    299.54 ns/iter   (244.97 ns … 1.14 µs) 289.41 ns 785.22 ns   1.14 µs
  • Deno
cpu: Apple M1 Max
runtime: deno 1.29.0 (aarch64-apple-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
new URL    350.18 ns/iter (342.22 ns … 374.91 ns) 351.44 ns 370.71 ns 374.91 ns

@Trott
Copy link
Member

Trott commented Dec 15, 2022

Maybe @cjihrig or @bnoordhuis could offer some information/advice on how to approach this.

@anonrig
Copy link
Member Author

anonrig commented Dec 15, 2022

Deno uses a smart way of just returning the start and end points for the states from native side: https://github.com/denoland/deno/blob/0d4e4af7acf82c1365999a7281910daa05f0e982/ext/url/lib.rs#L100

@cjihrig
Copy link

cjihrig commented Dec 15, 2022

Doesn't this just require someone to do some performance work on Node's URL implementation? I can't speak to Bun at all, but Deno has people that spend all (or most) of their time improving performance of things like this.

@jasnell
Copy link
Member

jasnell commented Dec 15, 2022

Yeah, the implementation was written to be spec compliant, not performant. Just needs some attention from folks who want to work on the performance tuning.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 15, 2022

We can maybe start from not regressing:

$ node benchmark/compare.js --old ~/.nvs/node/14.21.1/x64/bin/node --new ./node_main --filter url-parse --set method=whatwg --set type=wpt url > out.csv
$ node-benchmark-compare out.csv
                                                                                  confidence improvement accuracy (*)   (**)  (***)
url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='wpt' withBase='false'        ***    -40.63 %       ±3.07% ±4.09% ±5.32%
url/legacy-vs-whatwg-url-parse.js method='whatwg' e=1 type='wpt' withBase='true'         ***     10.59 %       ±4.53% ±6.02% ±7.84%

Be aware that when doing many comparisons the risk of a false-positive result increases.
In this case, there are 2 comparisons, you can thus expect the following amount of false-positive results:
  0.10 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.02 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

I'd also suggest that we need to be careful about micro-benchmarking the URL constructor using a constant input (e.g. our type=wpt benchmarks go to some lengths to make sure that the result is meaningful, and the input comes from the WPT test data set). There are a lot of factors that can affect the numbers (e.g. if you don't even use the returned result v8 might just fold things and you'd be comparing between noops and actually doing anything). So sheer numbers are not very meaningful when it's not clear if the benchmarks are written in the same and a proper way.

@anonrig
Copy link
Member Author

anonrig commented Jan 16, 2023

I'm working on this, and writing a new URL parser (available but work-in-progress at https://github.com/ada-url/ada)

@anonrig anonrig self-assigned this Jan 16, 2023
@tony-go
Copy link
Member

tony-go commented Jan 18, 2023

I'm working on this, and writing a new URL parser (available but work-in-progress at https://github.com/ada-url/ada)

ADA is neat ^^

@anonrig
Copy link
Member Author

anonrig commented Jan 23, 2023

A quick update referencing the benchmark that leads to this issue. For parsing the URL http://example.com, Ada takes 105ns.

---------------------------------------------------------------------------------
Benchmark                       Time             CPU   Iterations UserCounters...
---------------------------------------------------------------------------------
BasicBench_BoostURL           671 ns          671 ns       861030 time/byte=37.2652ns time/url=670.773ns url/s=1.49082M/s
BasicBench_CURL               496 ns          496 ns      1405227 time/byte=27.5795ns time/url=496.43ns url/s=2.01438M/s
BasicBench_AdaURL             105 ns          105 ns      6631614 time/byte=5.85535ns time/url=105.396ns url/s=9.48801M/s

@anonrig
Copy link
Member Author

anonrig commented Jan 29, 2023

Progress update on Ada: Right now, new URL is as fast as url.parse


> node-benchmarks@1.0.0 url
> ~/Developer/node/out/Release/node ./url/index.mjs && bun run ./url/index.mjs && deno run -A ./url/deno.js

cpu: Apple M1 Max
runtime: node v20.0.0-pre (arm64-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
new URL    490.83 ns/iter (478.33 ns … 524.51 ns) 494.07 ns 521.47 ns 524.51 ns
url.parse   461.8 ns/iter (455.77 ns … 506.29 ns) 461.78 ns 496.67 ns 506.29 ns
cpu: Apple M1 Max
runtime: bun 0.5.1 (arm64-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
new URL    293.58 ns/iter    (238.7 ns … 1.12 µs) 282.84 ns 826.27 ns   1.12 µs
url.parse    1.71 µs/iter      (1.6 µs … 2.19 µs)   1.79 µs   2.19 µs   2.19 µs
cpu: Apple M1 Max
runtime: deno 1.29.0 (aarch64-apple-darwin)

benchmark      time (avg)             (min … max)       p75       p99      p995
------------------------------------------------- -----------------------------
new URL    351.25 ns/iter (345.38 ns … 385.85 ns) 351.51 ns 381.79 ns 385.85 ns

@marvinhagemeister
Copy link
Contributor

Very happy to see this! The performance of new URL directly relates to module resolution too, when node is run in ESM mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants