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

utility: add benchmark library, but don't use it yet. #2350

Merged
merged 7 commits into from
Jan 17, 2018

Conversation

jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Jan 11, 2018

title: one line description

This is a prereq for running CI on #2348

Description:
Integrates github.com/google/benchmark so it can be used in future PRs for demonstrating improvement. Note this PR does not actually depend on it, so it can pass CI.

Risk Level: Low

Tested: //test/...

to the other handlers with a browser.

Add a private mechanism to have admin handlers supply arbitrary
headers.  Note that the handler callback doesn't expose a header
structure, so the public v2 API does not provide this header-control.
However, it was specifying text/html as content-type by default, so
I changed the default to text/plain (with nosniff).

Added sanitization of both the prefix string and the help text, which
need to be safely injected into the HTML doc.

Added some styling and a reference to the existing Envoy favicon as well.

Note that no existing handlers get any response bytes changed as a result
of this commit, just their content-type.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ms in particular.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
… can set the

content-type and/or cache-control.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
This is extracted from envoyproxy#2348
and needs to go in first -- with no code depdning on it -- so the CI
works in the PR which uses the new dependency.

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

Note: I tried for several hours to get this to work with genrule_repository and raw genrule before giving up. There don't appear to be any currently live examples of this flow working with a non-trivial C++ dependency, although @htuch pointed out there was one in the past:

genrule_repository(

But I had a hard time understanding how I would use this with an external github repo at a commit, as opposed to a versioned static file with a sha256.

I'm OK revisiting this later but right now this is something that IMO we need to fix a real perf issue.

@htuch
Copy link
Member

htuch commented Jan 11, 2018

@jmillikin-stripe can you provide an idea of what folks should be doing with genrule_repository? Alternatively, are you cool with use just using a build recipe here?

@jmillikin-stripe
Copy link
Contributor

But I had a hard time understanding how I would use this with an external github repo at a commit, as opposed to a versioned static file with a sha256.

genrule_repository doesn't know how to git clone, though we could teach it if needed. Using Github's archive tarballs would almost certainly be easier.

Alternatively, are you cool with use just using a build recipe here?

I'm fine in general with using build recipes, but note that this particular recipe has some issues (e.g. it creates its own, non-hermetic googletest checkout).

What are the chances that this official Google benchmark framework could grow a build config for use with Google's official build system...?

@jmarantz
Copy link
Contributor Author

The chance is pretty good we can get Bazel support: google/benchmark#501

However I don't want to wait for that :)

I was able to get a good hint from one of the devs and managed to remove the gtest dependency.

@jmarantz jmarantz closed this Jan 11, 2018
@jmarantz jmarantz reopened this Jan 11, 2018
Signed-off-by: Joshua Marantz <jmarantz@google.com>
@jmarantz
Copy link
Contributor Author

RE 'archive tarballs' -- how do they work? Is that something that has to be created by the repo owner?

@jmillikin-stripe
Copy link
Contributor

Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

Looks good.

@@ -0,0 +1,22 @@
#!/bin/bash

set -x
Copy link
Member

Choose a reason for hiding this comment

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

Remove -x (once done debug) and add -e.

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

cd ../benchmark

pwd
include_dir="$THIRDPARTY_BUILD/include/testing/base/public"
Copy link
Member

Choose a reason for hiding this comment

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

Prefer INCLUDE_DIR for consistency with other variables (I know in Google we use lowercase, but we would need to switch everything over).

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; FWIW I thought lower-case variables were common in bash, and upper-case variables when they are exported.

Copy link
Member

Choose a reason for hiding this comment

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

They are, and it's in the Google style guide, but we haven't had that convention in Envoy, so we tend to opt to be consistent with the existing code base as the lesser evil.

cp src/libbenchmark.a "$THIRDPARTY_BUILD"/lib
cd ../benchmark

pwd
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.

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

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

htuch commented Jan 17, 2018

@jmarantz I think you may have skipped the push after update.

@jmarantz
Copy link
Contributor Author

What's the next step for this PR...I am not (yet) a collaborator so I can't merge myself.

@htuch htuch merged commit 22e0d08 into envoyproxy:master Jan 17, 2018
@jmarantz jmarantz deleted the add-benchmark-library-only branch January 17, 2018 20:08
htuch pushed a commit that referenced this pull request Jan 23, 2018
Adds a benchmarking for new string utility functions added in #2368 using the google microbenchmarking library (https://github.com/google/benchmark), which was added in #2350. This serves as an example and proof of concept on how to use the benchmarking library, and as a reference point for the approximate performance of some string-parsing functions.

Risk Level: Very Low

Testing:
This is just a new speed-test, so running it on its own is sufficient to test it.

Results:

2018-01-18 08:01:44
Run on (12 X 3800 MHz CPU s)
CPU Caches:
  L1 Data 32K (x6)
  L1 Instruction 32K (x6)
  L2 Unified 256K (x6)
  L3 Unified 15360K (x1)
***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will incur extra overhead.
-------------------------------------------------------------------------------------
Benchmark                                              Time           CPU Iterations
-------------------------------------------------------------------------------------
BM_RTrimStringView                                    11 ns         11 ns   54467092
BM_RTrimStringViewAlreadyTrimmed                       9 ns          9 ns   77143046
BM_RTrimStringViewAlreadyTrimmedAndMakeString         24 ns         24 ns   29094696
BM_FindToken                                         165 ns        165 ns    4230849
BM_FindTokenValueNestedSplit                         427 ns        427 ns    1653627
BM_FindTokenValueSearchForEqual                      180 ns        180 ns    4046401

In practice I did not find this benchmark to be noisy on repeated runs.

Signed-off-by: Joshua Marantz <jmarantz@google.com>
Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020
Signed-off-by: Piotr Sikora <piotrsikora@google.com>
akonradi added a commit to akonradi/envoy that referenced this pull request Sep 16, 2020
The Google Test repository has endorsed Abseil's "live at head"
philosophy. Bump to a newer commit that includes google/googletest/envoyproxy#2350
for ease of writing tests.

Signed-off-by: Alex Konradi <akonradi@google.com>
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Signed-off-by: Mike Schore <mike.schore@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.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.

None yet

3 participants