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: remove problematic tls params #31816

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 16, 2020

These very small values can cause crashes/exceptions to occur on some systems because most time is spent in V8 GC or in parts of node core that are not being tested (e.g. streams).

Specifically, I was seeing size=1/size=2 for tls/secure-pair.js causing bench.end(0) occasionally (although it seems to have only started recently, but that could have been chance?).

While looking into that I also found that similarly small size values can cause crashing due to GC-related issues for tls/throughput.js.

I've also adjusted the tls benchmark test to use parameters that are faster.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem. labels Feb 16, 2020
@nodejs-github-bot
Copy link
Collaborator

@mscdex mscdex force-pushed the benchmark-tls-remove-slow-param branch from 9a9c963 to 00f1e42 Compare March 6, 2020 04:41
These very small values can cause crashes/exceptions to occur on some
systems because most time is spent in V8 GC or in parts of node core
that are not being tested (e.g. streams).

PR-URL: nodejs#31816
Reviewed-By: James M Snell <[email protected]>
@mscdex mscdex force-pushed the benchmark-tls-remove-slow-param branch from 00f1e42 to 5cc0754 Compare March 6, 2020 04:42
@mscdex mscdex merged commit 5cc0754 into nodejs:master Mar 6, 2020
@mscdex mscdex deleted the benchmark-tls-remove-slow-param branch March 6, 2020 04:44
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
These very small values can cause crashes/exceptions to occur on some
systems because most time is spent in V8 GC or in parts of node core
that are not being tested (e.g. streams).

PR-URL: #31816
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
codebytere pushed a commit that referenced this pull request Mar 16, 2020
These very small values can cause crashes/exceptions to occur on some
systems because most time is spent in V8 GC or in parts of node core
that are not being tested (e.g. streams).

PR-URL: #31816
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 17, 2020
These very small values can cause crashes/exceptions to occur on some
systems because most time is spent in V8 GC or in parts of node core
that are not being tested (e.g. streams).

PR-URL: #31816
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Mar 17, 2020
codebytere pushed a commit that referenced this pull request Mar 23, 2020
These very small values can cause crashes/exceptions to occur on some
systems because most time is spent in V8 GC or in parts of node core
that are not being tested (e.g. streams).

PR-URL: #31816
Reviewed-By: James M Snell <[email protected]>
codebytere pushed a commit that referenced this pull request Mar 30, 2020
These very small values can cause crashes/exceptions to occur on some
systems because most time is spent in V8 GC or in parts of node core
that are not being tested (e.g. streams).

PR-URL: #31816
Reviewed-By: James M Snell <[email protected]>
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. test Issues and PRs related to the tests. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants