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

benchmark: update iterations in benchmark/crypto/aes-gcm-throughput.js #50929

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

lucshi
Copy link

@lucshi lucshi commented Nov 27, 2023

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%

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem. labels Nov 27, 2023
@H4ad
Copy link
Member

H4ad commented Dec 2, 2023

I don't think that worth changing from 500 to 2500, I don't think this will trigger any optimization by V8.

@lucshi
Copy link
Author

lucshi commented Dec 2, 2023

I don't think that worth changing from 500 to 2500, I don't think this will trigger any optimization by V8.

Hi Vinicius,

The rational of this change is not to trigger JIT, but make the crypto code dominate the execution phase instead of fwrite. When the iteration was 500, most execution time was occupied by fwrite. When iterations was increased to 2500, most execution time would be occupied by crypto. To make the scoring reasonable, we need to increase the iterations. After some experiments I found 2500 is a proper value to avoid long execution time.

I attached the perf log of 500 and 2500 and please see if the change made sense. Thanks!

2500.perf.log
500.perf.log

Copy link
Member

@H4ad H4ad left a comment

Choose a reason for hiding this comment

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

In this case, make sense, thanks for the PR.

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 4, 2023
@H4ad H4ad added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 9, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 9, 2023
@nodejs-github-bot nodejs-github-bot merged commit 29af78e into nodejs:main Dec 9, 2023
30 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 29af78e

RafaelGSS pushed a commit that referenced this pull request Dec 15, 2023
Fixes: #50571
PR-URL: #50929
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Dec 15, 2023
richardlau pushed a commit that referenced this pull request Mar 25, 2024
Fixes: #50571
PR-URL: #50929
Refs: #50571
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants