Conversation
Convert ring hash creation and choose host benchmarks to the BM framework. This will set us up for comparing Maglev. Signed-off-by: Matt Klein <mklein@lyft.com>
|
Sample output: |
|
Was the benchmark sample run with "-c opt"? |
Yes. Ring hash build is very slow. :) |
jmarantz
left a comment
There was a problem hiding this comment.
really nice. Just some style questions for you to decide.
|
|
||
| HostVector hosts; | ||
| for (uint64_t i = 0; i < num_hosts; i++) { | ||
| hosts.push_back(makeTestHost(info_, fmt::format("tcp://10.0.{}.{}:6379", i / 256, i % 256))); |
There was a problem hiding this comment.
add assert that num_hosts<65536 above.
| hosts.push_back(makeTestHost(info_, fmt::format("tcp://10.0.{}.{}:6379", i / 256, i % 256))); | ||
| } | ||
| HostVectorConstSharedPtr updated_hosts{new HostVector(hosts)}; | ||
| host_set.updateHosts(updated_hosts, updated_hosts, nullptr, nullptr, hosts, {}); |
There was a problem hiding this comment.
wdyt of commenting the formal names for the nullptrs and {} ? I don't see this a lot in the codebase but it sometimes helps readability without having to refer back to the .h.
come to think of it, since updated_hosts is passed twice, indicating the formal for those actuals would be nice too.
up to you, obviously
There was a problem hiding this comment.
In general I feel that with a decent editor it doesn't add that much value, but happy to do it if preferable.
There was a problem hiding this comment.
Good point. I'll try to confer with others locally about how to get some kind of TAGS thing set up for Envoy. But it's not just the editor; it's also the code review tool, which ideally should have hot-links for all referenced types to their declaration, but github doesn't. Or is there a way to get github to light up some links in the review tool?
There was a problem hiding this comment.
Unfortunately I don't think GH does this.
There was a problem hiding this comment.
OK, np. It's not a big deal to poke around and find the formals when I need to know them; just an idea.
| state.PauseTiming(); | ||
| double mean = 0; | ||
| std::for_each(hit_counter.begin(), hit_counter.end(), | ||
| [&mean](const auto pair) { mean += pair.second; }); |
There was a problem hiding this comment.
In Envoy, is this preferred to
for (const auto pair : hit_counter) {
mean += pair.second;
}
There was a problem hiding this comment.
I just copied this code from elsewhere. Will fix.
| double variance = 0; | ||
| std::for_each(hit_counter.begin(), hit_counter.end(), [&variance, mean](const auto pair) { | ||
| variance += std::pow(pair.second - mean, 2); | ||
| }); |
modifying the NetworkConfigurationFilter to do asynchronous DNS lookups for non-IP-address proxies. If address resolution fails, the request will be short circuited by sendLocalReply rather than being sent upstream without being proxied. This PR also changes ConnectivityManager to have a pure API and an Impl for testing. Risk Level: low Testing: unit tests, integration test Docs Changes: n/a Release Notes: n/a part of #1622 fixes #2532 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
modifying the NetworkConfigurationFilter to do asynchronous DNS lookups for non-IP-address proxies. If address resolution fails, the request will be short circuited by sendLocalReply rather than being sent upstream without being proxied. This PR also changes ConnectivityManager to have a pure API and an Impl for testing. Risk Level: low Testing: unit tests, integration test Docs Changes: n/a Release Notes: n/a part of #1622 fixes #2532 Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: JP Simard <jp@jpsim.com>
Convert ring hash creation and choose host benchmarks to the BM framework.
This will set us up for comparing Maglev.
Risk Level: None
Testing: Test/benchmark code
Docs Changes: N/A
Release Notes: N/A