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: use much smaller values for n in some http tests #14002

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
6 participants
@psmarshall
Copy link
Contributor

commented Jun 30, 2017

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

benchmark

The n values here are really, really high right now, and we can still get high confidence (3 stars) with this much lower n value. This makes the benchmarks run a lot faster without sacrificing accuracy.

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2017

No comment on the changes themselves, but the commit subsystem should be benchmark, not test.

@jasnell

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

@psmarshall psmarshall force-pushed the psmarshall:test-n branch from a69350d to f5a431d Jul 24, 2017

@psmarshall psmarshall changed the title test: use much smaller values for n in some http tests benchmark: use much smaller values for n in some http tests Jul 24, 2017

@psmarshall

This comment has been minimized.

Copy link
Contributor Author

commented Jul 24, 2017

@cjihrig Thanks, I've updated the subsystem.

@BridgeAR
Copy link
Member

left a comment

I just checked the benchmarks and with these low numbers the benchmark is not fully limited by the CPU and the numbers change to what they would be with higher n.
It will depend on the CPU but I would not recommend to go much below 5e7. I normally always change n when I run benchmarks locally depending on if I want to have a high accuracy or a glimpse at the likely outcome.

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 8, 2017

Ping @psmarshall would you mind updating the numbers once more?

@BridgeAR

This comment has been minimized.

Copy link
Member

commented Sep 19, 2017

I decided to land this as is even with my comment. We can always increase the numbers when running the benchmarks locally if needed.

Landed in 8a968e4

@BridgeAR BridgeAR closed this Sep 19, 2017

BridgeAR added a commit that referenced this pull request Sep 19, 2017

benchmark: use smaller n value in some http tests
PR-URL: #14002
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Sep 20, 2017

benchmark: use smaller n value in some http tests
PR-URL: #14002
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017

benchmark: use smaller n value in some http tests
PR-URL: nodejs/node#14002
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017

benchmark: use smaller n value in some http tests
PR-URL: nodejs/node#14002
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins added a commit that referenced this pull request Oct 17, 2017

benchmark: use smaller n value in some http tests
PR-URL: #14002
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Oct 17, 2017

Merged

v6.12.0 proposal #16263

MylesBorins added a commit that referenced this pull request Oct 25, 2017

benchmark: use smaller n value in some http tests
PR-URL: #14002
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>

@MylesBorins MylesBorins referenced this pull request Nov 3, 2017

Merged

v4.8.6 proposal #16500

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.