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

completed secure-pair performance test with clear connections #27971

Closed
wants to merge 2 commits into from

Conversation

@tufosa
Copy link

tufosa commented May 30, 2019

A very simple patch to the existing secure-pair benchmark in order to include clear connections in the test and expose this issue

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
adds clear connections to the secure-pair performance test to prove
thah in some cases (when the sender send the data in small chunks)
clear connections perform worse than TLS connections

Refs: #27970
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented May 30, 2019

@tufosa Does it make sense to add a smaller chunk size to benchmark/net/net-pipe.js?

@tufosa

This comment has been minimized.

Copy link
Author

tufosa commented May 30, 2019

@addaleax yep!! Didn't know about this benchmark. Done in 809ee8c. The test with chunks of 2 bytes performs 100 times worse than the 64 bytes one.

@tufosa

This comment has been minimized.

Copy link
Author

tufosa commented Jul 17, 2019

@addaleax it's my first PR to this project and I am not sure how can we move this forward. Everything seems OK, but I don't know if I need to do anything else in order to get this reviewed and eventually approved. Can you help me with this?

@Trott Trott force-pushed the nodejs:master branch from 1ecc406 to 49cf67e Sep 17, 2019
@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Dec 18, 2019

ping @addaleax

@nodejs-github-bot

This comment has been minimized.

Copy link
Member

addaleax left a comment

@gireeshpunathil Anything in particular I’d need to be aware of here?

@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Dec 18, 2019

@addaleax - no, I pinged because I see this PR not moving and saw you as the only person engaged on this PR so far; so!

@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

gireeshpunathil added a commit that referenced this pull request Dec 20, 2019
adds clear connections to the secure-pair performance test to prove
that in some cases (when the sender send the data in small chunks)
clear connections perform worse than TLS connections

Also add a byte chunk size test to benchmark/net/net-pipe.js

Refs: #27970

PR-URL: #27971
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@gireeshpunathil

This comment has been minimized.

Copy link
Member

gireeshpunathil commented Dec 20, 2019

landed in fc553fd thanks for the contribution!

BridgeAR added a commit that referenced this pull request Jan 3, 2020
adds clear connections to the secure-pair performance test to prove
that in some cases (when the sender send the data in small chunks)
clear connections perform worse than TLS connections

Also add a byte chunk size test to benchmark/net/net-pipe.js

Refs: #27970

PR-URL: #27971
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@BridgeAR BridgeAR mentioned this pull request Jan 7, 2020
targos added a commit that referenced this pull request Jan 14, 2020
adds clear connections to the secure-pair performance test to prove
that in some cases (when the sender send the data in small chunks)
clear connections perform worse than TLS connections

Also add a byte chunk size test to benchmark/net/net-pipe.js

Refs: #27970

PR-URL: #27971
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.