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 <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Otherwise, non-mutating functions such as is_tree_granted unnecessarily require pointers to mutable data structures. PR-URL: nodejs#50705 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50723 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Deokjin Kim <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#50750 Refs: nodejs#50740 Refs: nodejs/reliability#718 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#50696 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Jiawen Geng <[email protected]>
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 <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: nodejs#50492 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Bryan English <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Stephen Belanger <[email protected]>
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 <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50772 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
In nodejs#50009, the return value was accidentally made part of `flush` option bullet point. PR-URL: nodejs#50760 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Deokjin Kim <[email protected]>
PR-URL: nodejs#50596 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#50712 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#50712 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#50712 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#50715 Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#50486 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
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 <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50829 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
PR-URL: nodejs#49868 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50640 Reviewed-By: Raz Luvaton <[email protected]> Reviewed-By: Moshe Atlow <[email protected]>
Fixes: nodejs#50818 PR-URL: nodejs#49793 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Jithil P Ponnan <[email protected]>
PR-URL: nodejs#50833 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: nodejs#50844 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
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 <[email protected]>
Commit-Queue: Chengzhong Wu (legendecas) <[email protected]>
Cr-Commit-Position: refs/heads/main@{#90398}
Refs: v8/v8@0f9ebbc
PR-URL: nodejs#50867
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs#50795 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50594 Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Fixes: nodejs#50741 Refs: nodejs#50444
PR-URL: nodejs#50816 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
- reduce copying by using std::move Signed-off-by: Michael Dawson <[email protected]> PR-URL: nodejs#50846 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50769 Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Qingyu Deng <[email protected]>
PR-URL: nodejs#50803 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50856 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
PR-URL: nodejs#50820 Reviewed-By: Jiawen Geng <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#50874 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#50881 Fixes: nodejs#50875 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
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 <[email protected]>
Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#50834 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruy Adorno <[email protected]>
PR-URL: nodejs#49846 Refs: nodejs/performance#106 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
PR-URL: nodejs#49884 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Santiago Gimeno <[email protected]> Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: James M Snell <[email protected]>
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%