-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
benchmark: pre-generate data set for URL benchmarks and use WPT test data #24302
Conversation
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js`
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case.
CI: https://ci.nodejs.org/job/node-test-pull-request/18529/ cc @TimothyGu @nodejs/url |
const bench = common.createBenchmark(main, { | ||
len: [1, 8, 16, 32, 64, 128], | ||
n: [1e6] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is n
being removed in general in this PR? If we're not going to be looping over the same input for each input, then we should at least loop n
times over the entire set of inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mscdex That should already been covered by the combination e
and --runs
(of the compare.js
): the size of the input is dependent on e
so to get a size of 1e6 we can just set e
to roughly Math.log(1e6 / 200, 2)
(all the data are pre-generated, so when e
is 1 and type
is long
it will iterate over the same kind of string 400 times. Only the type wpt
gives you a nonhomogenous array), but I don't think we actually need that many iterations for a single benchmark run, the n
was set when Crankshaft existed, and currently I don't observe a significance in the value of the n once it's large enough (by default it's roughly 800/400 with/without base). Instead we can just use --runs
to get more samples, but the default 30 is already a pretty good sample size (at least it's the magic number taught in statistics textbooks). So with the default setting for each input you run through the same code roughly 30 * 400 times with the same binary
@mscdex BTW this is the benchmark results I got for #24218 with this benchmark after setting See results
For comparison, with the old benchmark the accuracy isn't really any different from when See results
|
Added a type check for the argument of |
Ping @nodejs/url @nodejs/performance can I have some reviews please? As the data in #24302 (comment) demonstrates, this should not affect the accuracy of the benchmarks while providing more options for the data set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Since this affects benchmarks, let's run the benchmark CI job on it: https://ci.nodejs.org/job/node-test-commit-custom-suites/766/ |
Landed in 5f25dd1...8450d11, thanks! |
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: #24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@joyeecheung is this something you think should be backported to |
This patch: - Introduces `common.bakeUrlData` which can be used to pre-generate the data set for the URL benchmarks to loop through instead of looping over a constant. - Add the option to use WPT data in benchmarks for better diversity in the input - Add the option to benchmark URL parsing with base URLs (whatwg only) - Moves the data in `benchmark/fixtures/url-inputs.js` to `benchmark/common.js` PR-URL: nodejs#24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This patch adds the option in the create-clientrequest benchmark to accept URL inputs (as strings or as URL objects) so we can measure the impact of URL parsing in a more sophisticated use case. PR-URL: nodejs#24302 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
benchmark: pre-generate data set for URL benchmarks
This patch:
common.bakeUrlData
which can be used to pre-generatethe data set for the URL benchmarks to loop through instead of
looping over a constant.
in the input
benchmark/fixtures/url-inputs.js
tobenchmark/common.js
benchmark: support URL inputs in create-clientrequest
This patch adds the option in the create-clientrequest benchmark
to accept URL inputs (as strings or as URL objects) so we can
measure the impact of URL parsing in a more sophisticated use case.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes