benchmark: increase iteration number to appropriate one #50861
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Increase the iteration number from 500 to 2500 to make time consumption of crypto operation dominate the benchmark running. Fixes: nodejs#50571
nodejs-github-bot
added
benchmark
Increase the iteration number from 5000 to 500000 to make time consumption of crypto operation dominate the benchmark running. Fixes: nodejs#50571
H4ad
approved these changes
Nov 26, 2023
H4ad
added
author ready
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/50861 ✔ Done loading data for nodejs/node/pull/50861 ----------------------------------- PR info ------------------------------------ Title benchmark: increase iteration number to appropriate one (#50861) Author Lei Shi (@lucshi) Branch lucshi:my-branch -> nodejs:main Labels crypto, benchmark, author ready Commits 2 - benchmark: increase iteration number to appropriate one - benchmark: increase get-ciphers iteration number to appropriate one Committers 1 - Lei Shi PR-URL: https://github.com/nodejs/node/pull/50861 Refs: https://github.com/nodejs/node/issues/50571 Reviewed-By: Vinícius Lourenço Claro Cardoso ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/50861 Refs: https://github.com/nodejs/node/issues/50571 Reviewed-By: Vinícius Lourenço Claro Cardoso -------------------------------------------------------------------------------- ℹ This PR was created on Thu, 23 Nov 2023 02:51:43 GMT ✔ Approvals: 1 ✔ - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/50861#pullrequestreview-1749348026 ✘ This PR needs to wait 85 more hours to land (or 0 hours if there is one more approval) ✔ Last GitHub CI successful ℹ Green GitHub CI is sufficient -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/6995969572 |
H4ad
removed
author ready
Usages of `v8::ScriptOrModule` were removed in nodejs#44198 so the flag can be disabled by default. PR-URL: nodejs#50616 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com>
Otherwise, non-mutating functions such as is_tree_granted unnecessarily require pointers to mutable data structures. PR-URL: nodejs#50705 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
PR-URL: nodejs#50723 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
When --node-builtin-modules-path is used, we read and create new strings for builtins in each realm which increases the memory usage. As a result GC may not be able to keep up with the allocation done in the loop in the test. As a workaround, give GC a bit more time by waiting for a timer in the loop. PR-URL: nodejs#50735 Refs: nodejs#50726 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: nodejs#50750 Refs: nodejs#50740 Refs: nodejs/reliability#718 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#50696 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
It needs to copy the Node.js binary which is currently almost 100MB. To be safe, skip the test when the available disk space is smaller than 120MB. PR-URL: nodejs#50764 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Divide builtins into two lists depending on whether they are loaded before pre-execution or at run time, and give clearer suggestions about how to deal with them based on the category they are in. This helps preventing regressions like the one reported in nodejs#45662. PR-URL: nodejs#50708 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs#50492 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Bryan English <bryan@bryanenglish.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
This is a ESM benchmark, rewrite it so that we are directly benchmarking the ESM import.meta paths and using number of loads for op/s calculation, instead of doing it in startup benchmarks and nesting number of process/workers spawn for op/s calculation. PR-URL: nodejs#50683 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
PR-URL: nodejs#50772 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
In nodejs#50009, the return value was accidentally made part of `flush` option bullet point. PR-URL: nodejs#50760 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br> Reviewed-By: Deokjin Kim <deokjin81.kim@gmail.com>
PR-URL: nodejs#50596 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Current iteration number is too small that fwrite occupies large portion of execution time which made crypo execution time measured inaccurate. The iteration above 1e5 makes 50% higher and stable score. PR-URL: nodejs#50766 Reviewed-By: Debadree Chatterjee <debadree333@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
PR-URL: nodejs#50712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#50712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#50712 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
PR-URL: nodejs#50715 Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#50486 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
The permission model's security guarantees fall apart in the presence of relative symbolic links. When an application attempts to create a relative symlink, the permission model currently resolves the relative path into an absolute path based on the process's current working directory, checks whether the process has the relevant permissions, and then creates the symlink using the absolute target path. This behavior is plainly incorrect for two reasons: 1. The target path should never be resolved relative to the current working directory. If anything, it should be resolved relative to the symlink's location. (Of course, there is one insane exception to this rule: on Windows, each process has a current working directory per drive, and symlinks can be created with a target path relative to the current working directory of a specific drive. In that case, the relative path will be resolved relative to the current working directory for the respective drive, and the symlink will be created on disk with the resulting absolute path. Other relative symlinks will be stored as-is.) 2. Silently creating an absolute symlink when the user requested a relative symlink is wrong. The user may (or may not) rely on the symlink being relative. For example, npm heavily relies on relative symbolic links such that node_modules directories can be moved around without breaking. Because we don't know the user's intentions, we don't know if creating an absolute symlink instead of a relative symlink is acceptable. This patch prevents the faulty behavior by not (incorrectly) resolving relative symlink targets when the permission model is enabled, and by instead simply refusing the create any relative symlinks. The fs APIs accept Uint8Array objects for paths to be able to handle arbitrary file name charsets, however, checking whether such an object represents a relative part in a reliable and portable manner is tricky. Other parts of the permission model incorrectly convert such objects to strings and then back to an Uint8Array (see 1f64147), however, for now, this bug fix will simply throw on non-string symlink targets when the permission model is enabled. (The permission model already breaks existing applications in various ways, so this shouldn't be too dramatic.) PR-URL: nodejs#49156 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#50829 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs#49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#50640 Reviewed-By: Raz Luvaton <rluvaton@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Fixes: nodejs#50818 PR-URL: nodejs#49793 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jithil P Ponnan <jithil@outlook.com>
PR-URL: nodejs#50833 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>
PR-URL: nodejs#50844 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Original commit message:
[flags] Remove --harmony-string-is-well-formed
The String.prototype.isWellFormed and toWellFormed have shipped
since M111.
Bug: v8:13557
Change-Id: I27e332d2fde0f9ea8ad649c016a84d2d3e0bf592
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4931269
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Chengzhong Wu (legendecas) <legendecas@gmail.com>
Cr-Commit-Position: refs/heads/main@{#90398}
Refs: v8/v8@0f9ebbc
PR-URL: nodejs#50867
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: nodejs#50795 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#50594 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
c-ares has made intentional changes to the behavior of TXT records to comply with RFC 7208, which concatenates multiple strings for the same TXT record into a single string. Multiple TXT records are not concatenated. Also, response handling has changed, such that a response which is completely invalid in formatting is thrown away as a malicious forged/spoofed packet rather than returning EBADRESP. This is one step toward RFC 9018 (EDNS COOKIES) which will require the message to at least be structurally valid to validate against spoofed records. Fix By: Brad House (@bradh352) PR-URL: nodejs#50743 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Fixes: nodejs#50741 Refs: nodejs#50444
PR-URL: nodejs#50816 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Michael Dawson <midawson@redhat.com>
- reduce copying by using std::move Signed-off-by: Michael Dawson <midawson@redhat.com> PR-URL: nodejs#50846 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#50769 Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Qingyu Deng <i@ayase-lab.com>
PR-URL: nodejs#50803 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#50856 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruy Adorno <ruyadorno@google.com>
PR-URL: nodejs#50820 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#50874 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sometimes reporters link to a repo for their repro, which not only likely takes them more time to setup, but also is less convenient for maintainers. Setting up a repo goes against the idea of a minimal repro, as if it was actually minimal, they would not have bothered creating a repo. PR-URL: nodejs#50882 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Every other invocation of BN_bn2binpad checks the return value. For safety and consistency, do so in RandomPrimeTraits::EncodeOutput() as well. PR-URL: nodejs#50860 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs#50881 Fixes: nodejs#50875 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Increase the number of iterations from 1e5 to 5e6 to avoid the test performance gap caused by inactive V8 optimization caused by insufficient number of iterations Refs: nodejs#50571 PR-URL: nodejs#50698 Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Signed-off-by: Matteo Collina <hello@matteocollina.com> PR-URL: nodejs#50834 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruy Adorno <ruyadorno@google.com>
PR-URL: nodejs#49846 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
PR-URL: nodejs#49884 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: James M Snell <jasnell@gmail.com>
I will remove ciphers to make it clear. Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Increase the iteration number from 500 to 2500 to make time consumption of crypto operation dominate the benchmark running.
Refs: #50571
Below is score improvement from 500 to 2500, measured on Cascake server.
Improvement can be a bit more if changing to 5000 but the running time will be notable longer than appropriate duration.
To make it clear, I will submit more PRs one by one to address each case in crypto, and tracked by 50571 issue.
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" 554.5625838906785 n=2500 percent=144.46%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=134.25%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=136.22%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=151.78%
crypto/aes-gcm-throughput.js cipher="aes-128-gcm" n=2500 percent=125.49%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" 557.082005808399 n=2500 percent=131.03%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=131.05%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=136.30%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=149.98%
crypto/aes-gcm-throughput.js cipher="aes-192-gcm" n=2500 percent=123.60%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" 557.9763934240019 n=2500 percent=130.56%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=130.17%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=135.86%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=148.04%
crypto/aes-gcm-throughput.js cipher="aes-256-gcm" n=2500 percent=123.15%