Skip to content

stats: adds a speed-test for accumulating http response stats#4998

Merged
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
jmarantz:codes-speed-test
Nov 9, 2018
Merged

stats: adds a speed-test for accumulating http response stats#4998
mattklein123 merged 3 commits intoenvoyproxy:masterfrom
jmarantz:codes-speed-test

Conversation

@jmarantz
Copy link
Copy Markdown
Contributor

@jmarantz jmarantz commented Nov 8, 2018

Description: This serves as a baseline for the speed improvements offered in #4997 . With this PR, on my machine, I get:

---------------------------------------------------------
Benchmark                  Time           CPU Iterations
---------------------------------------------------------
BM_AddResponses         8220 ns       8220 ns      80376
BM_ResponseTiming        447 ns        447 ns    1575300

which appears to be fairly consistent, at least localized in time.

Risk Level: low -- just adds a new speed test
Testing: just this test
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Nice, great place to start. Obviously if you eventually do an optimization around common HTTP codes we might need to make this a bit more complex, but seems good for now.

}
BENCHMARK(BM_ResponseTiming);

// Boilerplate main(), which discovers benchmarks in the same file and runs them.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As an aside, I feel like we have this now in a lot of different files. Is there anyway to make this a little less copy/paste?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

agreed -- should be able to just put that main() into a new source/common/common/benchmark.h, though in some case you may need a global init (because statics). Follow-up OK? I'd do that across all the speed-tests in our repo.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yup follow up fine!

@mattklein123 mattklein123 merged commit 695dd70 into envoyproxy:master Nov 9, 2018
@jmarantz jmarantz deleted the codes-speed-test branch November 9, 2018 02:21
fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
…roxy#4998)

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to 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