Skip to content

tests: refactor benchmarks to remove test server overhead - #20

Merged
felixge merged 1 commit into
felixge:masterfrom
costela:refactor-benchmarks-to-remove-test-server-overhead
Jul 24, 2022
Merged

tests: refactor benchmarks to remove test server overhead#20
felixge merged 1 commit into
felixge:masterfrom
costela:refactor-benchmarks-to-remove-test-server-overhead

Conversation

@costela

Copy link
Copy Markdown
Contributor

The httptest Server introduces unrelated overhead to the benchmarks.
Ideally we'd only measure the impact on the wrapped handler.

Related to #19

The httptest Server introduces unrelated overhead to the benchmarks.
Ideally we'd only measure the impact on the wrapped handler.
@costela
costelaforce-pushed the refactor-benchmarks-to-remove-test-server-overhead branch from cbf8594 to 0db2505CompareJuly 20, 2022 16:29
@costelacostela mentioned this pull request Jul 21, 2022
@felixge
felixge merged commit 8b7a371 into felixge:masterJul 24, 2022
@felixge

Copy link
Copy Markdown
Owner

🙏 Thanks. I think I originally put the test server into the benchmark so that the overhead created by httpsnoop is "put into perspective". After applying your patch people might think that httpsnoop is causing 240x overhead 🙈. Anyway, I agree that it's more useful to exclude the test server overhead while trying to optimize things. So I'm okay with this change.

$ go test -bench .
goos: darwin
goarch: amd64
pkg: github.com/felixge/httpsnoop
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkBaseline-12 809914038 1.479 ns/op 0 B/op 0 allocs/op
BenchmarkCaptureMetrics-12 3394172 353.9 ns/op 225 B/op 7 allocs/op
BenchmarkCaptureMetricsTwice-12 1715709 698.2 ns/op 450 B/op 14 allocs/op
BenchmarkWrap-12 8871846 133.1 ns/op
PASS
ok github.com/felixge/httpsnoop	6.244s

@costela

Copy link
Copy Markdown
ContributorAuthor

Yes, that's a very good point. But I'd argue that this perspective is best given in a README, if these results are mentioned somewhere. If we're running the benchmarks while working on the code, this added "context" is mostly implicit?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@costela@felixge