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

utlity: Add benchmark for string-utility functions #2348

Merged
merged 18 commits into from
Jan 23, 2018
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
2804d6c
Add an HTML home page for the admin port, which makes it easier to surf
jmarantz Dec 21, 2017
8eb9e25
Add unit tests and get all tests working, correcting some path proble…
jmarantz Dec 22, 2017
29bc713
Make the admin HandlerCb pass through a headers reference so handlers…
jmarantz Dec 22, 2017
8fe318b
Merge remote-tracking branch 'refs/remotes/origin/master'
jmarantz Jan 3, 2018
5e5e8a4
Adds Google benchmark infrastructure, compiled externally via cmake.
jmarantz Jan 11, 2018
b79d16a
Shortens rightTrim function.
jmarantz Jan 11, 2018
70b105d
Removes string_review rightTrim impl in utility.cc, which should reve…
jmarantz Jan 11, 2018
be822f4
Remove absl includes now that I reverted the references to it.
jmarantz Jan 11, 2018
fcdfe13
Fix braces nit in boilerplate main code. Remove excess printing.
jmarantz Jan 11, 2018
daf4d41
Add external_deps support to envoy_cc_binary and remove the intermedi…
jmarantz Jan 11, 2018
9057266
Remove gtest dependency from benchmark.
jmarantz Jan 11, 2018
c94cf1d
Merge branch 'master' into add-benchmark-library
jmarantz Jan 17, 2018
dc1db43
Merge branch 'master' into add-benchmark-library
jmarantz Jan 17, 2018
5132ae7
Remove some detritus from earlier PRs and stats_speed_test, which is …
jmarantz Jan 17, 2018
bcf5812
Update the SHA to hopefully get CI to work.
jmarantz Jan 18, 2018
b22ea04
Add an alternative faster implementation for finding a value in a token.
jmarantz Jan 18, 2018
dda3cda
Add an alternative faster findToken operation.
jmarantz Jan 19, 2018
12b97c6
formatting fix-up.
jmarantz Jan 23, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bazel/external/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ cc_library(
name = "all_external",
srcs = [":empty.cc"],
deps = [
"@io_opentracing_cpp//:opentracing",
"@com_lightstep_tracer_cpp//:lightstep_tracer",
"@com_google_googletest//:gtest",
"@com_lightstep_tracer_cpp//:lightstep_tracer",
"@io_opentracing_cpp//:opentracing",
],
)

Expand Down
1 change: 1 addition & 0 deletions bazel/target_recipes.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# ci/build_container/build_recipes.
TARGET_RECIPES = {
"ares": "cares",
"benchmark": "benchmark",
"event": "libevent",
"event_pthreads": "libevent",
"tcmalloc_and_profiler": "gperftools",
Expand Down
21 changes: 21 additions & 0 deletions ci/build_container/build_recipes/benchmark.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#!/bin/bash

set -x

export COMMIT="e1c3a83b8197cf02e794f61228461c27d4e78cfb" # benchmark @ Jan 11, 2018
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll continue to iterate on that.


git clone https://github.com/google/benchmark.git
(cd benchmark; git reset --hard "$COMMIT")
git clone https://github.com/google/googletest.git benchmark/googletest
mkdir build

cd build
cmake -G "Unix Makefiles" ../benchmark -DCMAKE_BUILD_TYPE=RELEASE
make
cp src/libbenchmark.a "$THIRDPARTY_BUILD"/lib
cd ../benchmark

pwd
include_dir="$THIRDPARTY_BUILD/include/testing/base/public"
mkdir -p "$include_dir"
cp include/benchmark/benchmark.h "$include_dir"
7 changes: 7 additions & 0 deletions ci/prebuilt/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ cc_library(
includes = ["thirdparty_build/include"],
)

cc_library(
name = "benchmark",
srcs = ["thirdparty_build/lib/libbenchmark.a"],
hdrs = ["thirdparty_build/include/testing/base/public/benchmark.h"],
includes = ["thirdparty_build/include"],
)

cc_library(
name = "crypto",
srcs = ["thirdparty_build/lib/libcrypto.a"],
Expand Down
1 change: 1 addition & 0 deletions source/common/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ envoy_cc_library(
name = "utility_lib",
srcs = ["utility.cc"],
hdrs = ["utility.h"],
external_deps = ["abseil_strings"],
deps = ["//include/envoy/common:time_interface"],
)

Expand Down
5 changes: 5 additions & 0 deletions source/common/common/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,11 @@ void StringUtil::rtrim(std::string& source) {
}
}

absl::string_view StringUtil::rightTrim(absl::string_view source) {
Copy link
Member

@gsagula gsagula Jan 11, 2018

Choose a reason for hiding this comment

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

@jmarantz I created a new class StringViewUtil with similar method/implementation in my last commit. Check this out: https://github.com/gsagula/envoy/blob/02ba40dd236645b89aec7689a81c9ff89f71dfc2/source/common/common/utility.cc#L222

Comment:
I think it's important to explicitly differentiate methods that use string view. Let me know what you think, so I might just move everything under StringUtil instead.

Copy link
Contributor Author

@jmarantz jmarantz Jan 11, 2018

Choose a reason for hiding this comment

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

In absl -- and in earlier versions of similar libraries in Google & Chromium, they are co-mingled. Note that absl::string_view is I think intended as a stop-gap until C++17, when std::string_view (http://en.cppreference.com/w/cpp/string/basic_string_view) becomes available.

The reason -- I think -- that they are co-mingled is that there's no reason for the ones that take std::string arguments to exist at all. E.g. in my example if you want to right-strip a std::string you can just pass in it, because absl::string_view has an implicit constructor from std::string, though it requires an explicit std::string out-conversion, e.g.:

    std::string foo;
    std::string bar = std::string(StringUtil::rightStrip(foo));

Ordinarily, though, if the backing-store stays alive along enough, you can just live in the string_view world and never make copies.

See https://github.com/abseil/abseil-cpp/blob/master/absl/strings/strip.h where the doc speaks in terms of std::string but actually all the functions take string_view.

So my preference is that you replace the ones in StringUtil:: rather than adding new alternatives. This will help spread the knowledge about string_view's performance by eliminating the slower equivalent.

Having said that, I knew your PR was coming, and the only reason I added my own version of rightStrip was as a testing vehicle for the benchmarking integration. And I want the benchmarking for something unrelated (faster startup in the presence of giant configs). Do you think you could split your StringUtil stuff in your PR into a smaller one, in which you also eliminate entirely the old rtrim and fix its call-sites? Unfortunately the rtrim function's signature (modifying a std::string&) can't be fixed in place.

What I'll do in any case is move my new utility function into the benchmark code itself, and your version can be authoritative, and I'll have a TODO on mine to delete it it and call yours instead once it's in.

Copy link
Member

Choose a reason for hiding this comment

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

@jmarantz Thanks for the details.

Sounds good to me. I will open a small PR for StringUtil refactoring and some new methods that I need for #2087

source.remove_suffix(source.size() - source.find_last_not_of(" \t\f\v\n\r") - 1);
return source;
}

size_t StringUtil::strlcpy(char* dst, const char* src, size_t size) {
strncpy(dst, src, size - 1);
dst[size - 1] = '\0';
Expand Down
7 changes: 7 additions & 0 deletions source/common/common/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

#include "envoy/common/time.h"

#include "absl/strings/string_view.h"

namespace Envoy {
/**
* Utility class for formatting dates given a strftime style format string.
Expand Down Expand Up @@ -136,6 +138,11 @@ class StringUtil {
*/
static void rtrim(std::string& source);

/**
* Trim trailing whitespace from a string_view, returning result.
*/
static absl::string_view rightTrim(absl::string_view source);

/**
* Size-bounded string copying and concatenation
*/
Expand Down
19 changes: 19 additions & 0 deletions test/common/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ licenses(["notice"]) # Apache 2

load(
"//bazel:envoy_build_system.bzl",
"envoy_cc_binary",
"envoy_cc_library",
Copy link
Member

Choose a reason for hiding this comment

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

Not needed.

"envoy_cc_test",
"envoy_package",
)
Expand Down Expand Up @@ -69,3 +71,20 @@ envoy_cc_test(
srcs = ["callback_impl_test.cc"],
deps = ["//source/common/common:callback_impl_lib"],
)

envoy_cc_library(
name = "utility_speed_test_lib",
srcs = ["utility_speed_test.cc"],
external_deps = [
"abseil_strings",
"benchmark",
],
deps = [
"//source/common/common:utility_lib",
],
)

envoy_cc_binary(
name = "utility_speed_test",
deps = ["utility_speed_test_lib"],
)
Copy link
Member

Choose a reason for hiding this comment

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

How come the binary/lib split here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

envoy_cc_binary does not allow an external lib. I'll add that as a comment.

Copy link
Member

Choose a reason for hiding this comment

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

You could also extend envoy_cc_binary to do this, it's only a couple of lines in the .bzl if you look at what envoy_cc_library does.

48 changes: 48 additions & 0 deletions test/common/common/utility_speed_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
#include <iostream>

#include "common/common/utility.h"

#include "testing/base/public/benchmark.h"

static const char TextToTrim[] = "\t the quick brown fox jumps over the lazy dog\n\r\n";
static size_t TextToTrimLength = sizeof(TextToTrim) - 1;

// NOLINT(namespace-envoy)

static void printResults(int accum, int iters) {
std::cout << "avg trimmed=" << static_cast<float>(accum) / iters << " of " << iters << " iters."
<< std::endl;
}

static void BM_RTrimString(benchmark::State& state) {
int accum = 0;
int iters = 0;
for (auto _ : state) {
std::string text(TextToTrim, TextToTrimLength);
Envoy::StringUtil::rtrim(text);
accum += TextToTrimLength - text.size();
++iters;
}
printResults(accum, iters);
}
BENCHMARK(BM_RTrimString);

static void BM_RTrimStringView(benchmark::State& state) {
int accum = 0;
int iters = 0;
for (auto _ : state) {
absl::string_view text(TextToTrim, TextToTrimLength);
text = Envoy::StringUtil::rightTrim(text);
accum += TextToTrimLength - text.size();
++iters;
}
printResults(accum, iters);
}
BENCHMARK(BM_RTrimStringView);

int main(int argc, char** argv) {
benchmark::Initialize(&argc, argv);
if (benchmark::ReportUnrecognizedArguments(argc, argv))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: braces on even single line if preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done; this was just a c&p from the example in benchmark.h, but I added the braces.

return 1;
benchmark::RunSpecifiedBenchmarks();
}
5 changes: 5 additions & 0 deletions test/common/common/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ TEST(StringUtil, rtrim) {
}
}

TEST(StringUtil, rtrimView) {
EXPECT_EQ("", StringUtil::rightTrim(" "));
EXPECT_EQ(" hello", StringUtil::rightTrim(" hello \r\t\r\n"));
}

TEST(StringUtil, strlcpy) {
{
char dest[6];
Expand Down