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.

Sign up for GitHub

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

Jump to bottom

benchmark: remove deprecated tls.createSecurePair #31785

Closed
wants to merge 1 commit into from
Closed

benchmark: remove deprecated tls.createSecurePair #31785

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 14, 2020

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 14, 2020
bnoordhuis
bnoordhuis approved these changes Feb 14, 2020
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

While the PR itself LGTM, my question is: why? The benchmark is useful for comparing SecurePair with TLSSocket.

@mscdex
Copy link
Contributor Author

mscdex commented Feb 14, 2020 •
edited
Loading

tls.createSecurePair() is deprecated and it doesn't seem to be useful to benchmark against something that is going to be removed (which could also mean the code won't be changed anyway so comparing performance with it doesn't seem useful in the long run). Additionally it reduces the amount of time taken to run the tls benchmarks.

@jasnell
Copy link
Member

jasnell commented Feb 14, 2020

I'm +0 on this one. Keeping the benchmark until (and if) we EOL SecurePair seems fine

addaleax
addaleax requested changes Feb 16, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

I’m -1 on this.

The benchmarks were even added after the deprecation, intentionally, to compare the overhead of the implementations.

I’m also not agreeing that tls.createSecurePair() is going to be removed.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@mscdex mscdex closed this Jun 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax requested changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mscdex @jasnell @bnoordhuis @addaleax @nodejs-github-bot