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

test: fix long running benchmark tests #20125

Closed
wants to merge 8 commits into from
Closed

test: fix long running benchmark tests #20125

wants to merge 8 commits into from

Conversation

apapirovski
Copy link
Member

This is a collection of changes that improve the execution time of the benchmark tests. Some are fixing missing parameters, others are slightly adjusting the benchmarks themselves, etc.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

@apapirovski apapirovski added test Issues and PRs related to the tests. benchmark Issues and PRs related to the benchmark subsystem. labels Apr 18, 2018
@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. http Issues or PRs related to the http subsystem. test Issues and PRs related to the tests. v8 engine Issues and PRs related to the V8 dependency. labels Apr 18, 2018
@apapirovski
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/14359/

@BridgeAR BridgeAR mentioned this pull request Apr 18, 2018
Closed
@BridgeAR
Copy link
Member

@apapirovski would you be so kind and check if the sequential benchmarks are now fast enough to be moved into parallel? :-)

@apapirovski apapirovski removed v8 engine Issues and PRs related to the V8 dependency. buffer Issues and PRs related to the buffer subsystem. http Issues or PRs related to the http subsystem. labels Apr 19, 2018
@apapirovski apapirovski mentioned this pull request Apr 21, 2018
Closed
2 tasks
@Trott
Copy link
Member

Trott commented Apr 21, 2018

@nodejs/benchmarking (not sure if the changes to the actual benchmark code might have unforeseen adverse impact on, say, rendering of graphs at https://benchmarking.nodejs.org/)

@Trott
Copy link
Member

Trott commented Apr 21, 2018

@nodejs/benchmarking (not sure if the changes to the actual benchmark code might have unforeseen adverse impact on, say, rendering of graphs at https://benchmarking.nodejs.org/)

Looking more closely, that seems unlikely, although it would still be great to get a review from that team.

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 21, 2018
@Trott
Copy link
Member

Trott commented Apr 21, 2018 •
edited
Loading

node-test-commit-linuxone rerun: https://ci.nodejs.org/job/node-test-commit-linuxone/579/

@Trott
Copy link
Member

Trott commented Apr 22, 2018

@apapirovski would you be so kind and check if the sequential benchmarks are now fast enough to be moved into parallel? :-)

@BridgeAR There's only two modified in this PR.

One is http which can't be moved to parallel because it uses predefined ports in the benchmarks.

The other is buffer. That one is a pure refactor. This change should not affect its execution time. (This PR adds two options to the test. Those options are added to the benchmark in this PR. Net effect on run time should be approximately zero.)

@apapirovski
Copy link
Member Author

This PR adds two options to the test. Those options are added to the benchmark in this PR. Net effect on run time should be approximately zero.)

To be fair, it adds those options in favour of what were currently large defaults. So it actually does affect execution time but not enough that it should go in parallel. I think it was only 10-15% faster.

@Trott
Copy link
Member

Trott commented Apr 22, 2018

To be fair, it adds those options in favour of what were currently large defaults. So it actually does affect execution time but not enough that it should go in parallel. I think it was only 10-15% faster.

Ah! Yes, good point. Thanks for the correction.

@apapirovski
Copy link
Member Author

Landed in e5f5320...f48ca9c

@apapirovski apapirovski deleted the fix-long-running-tests branch April 22, 2018 09:08
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: fix long-running buffer benchmarks
a2cf005 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: fix long-running http benchmarks
957532e 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
benchmark: do not multiply n by 1e6 in arrays
71cc57e 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: use correct arg name in assert benchmark
967d805 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: use correct arg name in domains benchmark
feba43c 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: fix missing param in url benchmark
e7913e4 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: reduce duration in misc benchmark
6af4f5e 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
apapirovski added a commit that referenced this pull request Apr 22, 2018
@apapirovski
test: fix arg names in benchmark string_decoder
f48ca9c 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: fix long-running buffer benchmarks
c61db33 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: fix long-running http benchmarks
6278c4b 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
benchmark: do not multiply n by 1e6 in arrays
f572927 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: use correct arg name in assert benchmark
8c3e672 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: use correct arg name in domains benchmark
77dc257 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: fix missing param in url benchmark
26a0a46 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: reduce duration in misc benchmark
6c90aee 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
jasnell pushed a commit that referenced this pull request Apr 23, 2018
@apapirovski @jasnell
test: fix arg names in benchmark string_decoder
bc6965b 
PR-URL: #20125
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@apapirovski @BridgeAR @Trott @nodejs-github-bot