buffer: use std::to_chars for serializing doubles by jmarantz · Pull Request #29500 · envoyproxy/envoy

jmarantz · 2023-09-07T21:16:39Z

Commit Message: A performance regression was found in a late commit as part of #28988 where we switched from a low-precision fast serialization of doubles (absl::StrCat) to a high-precision slow serialization of doubles (absl::StrFormat("%.15g", ...). This resolves that issue using std::to_chars which is faster than absl::StrCat and is also high precision. The downside is it doesn't work on all platforms, so an intermediate high-precision medium-speed solution was found for Apple and GCC. The serialize-double-to-buffer functionality is moved out of Json::Streamer and into a new Buffer::Util namespace.

Latest Json histogram serialization performance:

------------------------------------------------------------------------
Benchmark                              Time             CPU   Iterations
------------------------------------------------------------------------
BM_HistogramsJson                   17.3 ms         17.3 ms           41

Additional Description:
Risk Level: low
Testing: /test/common/json/... test/common/buffer //test/server/admin/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2023-09-07T22:11:36Z

source/server/admin/stats_render.cc

  for (const Stats::ParentHistogram::Bucket& bucket : buckets) {
-    response.addFragments(
-        {delim, absl::StrCat(bucket.lower_bound_, ",", bucket.width_, ":", bucket.count_)});
+    response.addFragments({delim, absl::StrFormat("%.15g,%.15g:%lu", bucket.lower_bound_,


Note: I did not use the new mechanism to serialize a double in this case because it did not make the benchmark run faster, but did make the code more verbose and harder to read. However I realized we were dropping precision on the text histogram so I wanted to fix it.

source/server/admin/stats_render.cc

jmarantz · 2023-09-08T03:30:29Z

/retest

jmarantz · 2023-09-08T12:56:19Z

/retest

moderation · 2023-09-08T16:19:08Z

On Linux x86_64 I'm getting build errors

ERROR: /home/moderation/Library/envoyproxy/envoy/source/common/buffer/BUILD:46:17: Compiling source/common/buffer/buffer_util.cc failed: (Exit 1): clang-14 failed: error executing command (from target //source/common/buffer:buffer_util_lib) /opt/llvm/bin/clang-14 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer -g0 -O2 ... (remaining 144 arguments skipped)
source/common/buffer/buffer_util.cc:32:33: error: no matching function for call to 'to_chars'
  std::to_chars_result result = std::to_chars(buf, buf + sizeof(buf), number);
                                ^~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/charconv:371:5: note: candidate template ignored: requirement '__and_<std::is_integral<double>, std::__not_<std::__or_<std::is_same<double, bool>, std::is_same<double, char16_t>, std::is_same<double, char32_t>, std::is_same<double, wchar_t>>>>::value' was not satisfied [with _Tp = double]
    to_chars(char* __first, char* __last, _Tp __value, int __base = 10)
    ^
1 error generated.

Is compiling fine on MacOS aarch64. Still building on Linux aarch64. Edit - this is using Google RBE /cc @jmarantz

jmarantz · 2023-09-08T17:16:22Z

interesting; it did pass CI. Can you tell from your logs exactly what compiler was being used?

jmarantz · 2023-09-08T17:23:55Z

It also passes for me with RBE using command

bazelisk --bazelrc=/usr/local/google/home/jmarantz/.bazelrc.remote test
  --//source/extensions/wasm_runtime/v8:enabled=false \
  --config=remote \
   //test/common/buffer/...

Where my .bazelrc.remote has these lines:

# GCP remote cache
build --remote_instance_name=projects/envoy-rbe/instances/default_instance
build --remote_cache=grpcs://remotebuildexecution.googleapis.com

# GCP remote execution (change 'build' to 'build:remote' to use local defaults).
#build:remote --remote_executor=grpcs://remotebuildexecution.googleapis.com
#build:remote --jobs=200
#build:remote --config=rbe-toolchain-clang-libc++
build --remote_executor=grpcs://remotebuildexecution.googleapis.com
build --jobs=200
build --config=rbe-toolchain-clang-libc++

I'm wondering if somehow your toolchain is not quite consistent with what's in CI or my RBE.

moderation · 2023-09-08T17:24:33Z

Checking now but have been building with my setup for ages without issues

jmarantz · 2023-09-08T17:26:38Z

Well this PR does add some dependency on the C++ runtime supporting to_chars with a double. On Apple and in GCC this is not the case and a different strategy is selected via ifdef.

So I suspect you have a version of clang setup somehow which does not have that to_chars support. Can you tell me what version you are using? We could add another ifdef clause, but as CI is working I'd prefer not rolling back.

moderation · 2023-09-08T17:30:59Z

If you scroll in the error is says it is using clang 14.

I'm invoking my RBE build with
bazel build -c opt //source/exe:envoy-static.stripped --config=remote-clang --remote_cache=grpcs://remotebuildexecution.googleapis.com --remote_executor=grpcs://remotebuildexecution.googleapis.com --google_credentials=<redacted> --jobs=64 --ui_actions_shown=64

This is a super old invocation I put together with Matt and Harvey when I was onboarded to RBE. Will try some other options.

Update - looks like my invocation was old and --config=rbe-toolchain-clang-libc++ fixes the issue. Thanks!

jmarantz · 2023-09-08T18:47:18Z

great. I guess maybe your library was from an earlier version of clang and this PR makes it more sensitive to that.

If this continues to be a paper-cut I'll write a PR to test library-version >= 14 and use the slower serializer for earlier libraries, but it will be annoying to test :)

jbohanon · 2023-09-09T01:14:27Z

I am building locally in the devcontainer, not using RBE and began receiving this error after rebasing onto this commit. The cxxopt was only added because I saw '-std=c++0x' in the error message, but it didn't help the issue, and the error was the same without it.

bazel test -c dbg //test/extensions/filters/http/ext_proc:ext_proc_integration_test --test_arg="--gtest_filter=*TestDownstreamProtocolError*" --jobs=18 --test_env=ENVOY_IP_TEST_VERSIONS=v4only --test_strategy=exclusive --cxxopt='-std=c++17'
INFO: Invocation ID: 3ec30554-fc5f-476c-a2e2-0c9037aba643
INFO: Build option --cxxopt has changed, discarding analysis cache.
INFO: Analyzed target //test/extensions/filters/http/ext_proc:ext_proc_integration_test (0 packages loaded, 33543 targets configured).
INFO: Found 1 test target...
ERROR: /workspaces/envoy/source/common/buffer/BUILD:46:17: Compiling source/common/buffer/buffer_util.cc failed: (Exit 1): clang-14 failed: error executing command (from target //source/common/buffer:buffer_util_lib) /opt/llvm/bin/clang-14 -U_FORTIFY_SOURCE -fstack-protector -Wall -Wthread-safety -Wself-assign -Wunused-but-set-parameter -Wno-free-nonheap-object -fcolor-diagnostics -fno-omit-frame-pointer '-std=c++0x' ... (remaining 140 arguments skipped)

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
source/common/buffer/buffer_util.cc:32:33: error: no matching function for call to 'to_chars'
  std::to_chars_result result = std::to_chars(buf, buf + sizeof(buf), number);
                                ^~~~~~~~~~~~~
/usr/lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/charconv:371:5: note: candidate template ignored: requirement '__and_<std::is_integral<double>, std::__not_<std::__or_<std::is_same<double, bool>, std::is_same<double, char16_t>, std::is_same<double, char32_t>, std::is_same<double, wchar_t>>>>::value' was not satisfied [with _Tp = double]
    to_chars(char* __first, char* __last, _Tp __value, int __base = 10)
    ^
1 error generated.
Target //test/extensions/filters/http/ext_proc:ext_proc_integration_test failed to build
Use --verbose_failures to see the command lines of failed build steps.
INFO: Elapsed time: 201.837s, Critical Path: 20.46s
INFO: 1272 processes: 23 internal, 1249 processwrapper-sandbox.
FAILED: Build did NOT complete successfully
//test/extensions/filters/http/ext_proc:ext_proc_integration_test FAILED TO BUILD

Executed 0 out of 1 test: 1 fails to build.

edit: interesting that while it is invoking clang-14, we can see that the std lib code is in /usr/lib/gcc...

jbohanon · 2023-09-09T02:08:46Z

^ this is resolved by running sudo apt-get install -y libstdc++-11-dev in the dev container. 9 (pre-installed) fails with the above shown error, and 10 fails with source/common/buffer/buffer_util.cc:32:33: error: call to 'to_chars' is ambiguous

I'm not sure which minimum version of libc++ works, but currently the bazel/setup_clang.sh script sets up the --config=clang option and the recommended user.bazelrc makes this the default. Devs are instructed in this guide that this setup means using clang with libstdc++.

I will open a PR to add libstdc++-11-dev to the devcontainer and update that guide to specify libstdc++-11-dev instead of libstdc++-7-dev

jmarantz · 2023-09-09T14:32:50Z

@jbohanon spoiied the surprise but assuming #29532 passes I'll try to get that merged so that this buffer-serialization improvement does not impact people's dev setups, which apparently have a variety of compiler/library combos not tested in CI.

use std::to_chars for serializing doubles.

b6e5f4d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Sep 7, 2023

stream out histogram fragments to HTML #29356

Merged

change ASSERT to ENVOY_BUG though it does carry a small perf penalty.

1614229

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz assigned ggreenway Sep 7, 2023

jmarantz commented Sep 7, 2023

View reviewed changes

ggreenway reviewed Sep 7, 2023

View reviewed changes

source/server/admin/stats_render.cc Show resolved Hide resolved

ggreenway approved these changes Sep 7, 2023

View reviewed changes

jmarantz merged commit 2cb9401 into envoyproxy:main Sep 8, 2023

jmarantz deleted the serialize-double-fast-accurate branch September 8, 2023 13:42

jbohanon mentioned this pull request Sep 9, 2023

buffer: use compiler version rather than other platform ifdefs to determine library call to use to serialize doubles #29532

Merged

Conversation

jmarantz commented Sep 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Sep 8, 2023

Uh oh!

jmarantz commented Sep 8, 2023

Uh oh!

moderation commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Sep 8, 2023

Uh oh!

jmarantz commented Sep 8, 2023

Uh oh!

moderation commented Sep 8, 2023

Uh oh!

jmarantz commented Sep 8, 2023

Uh oh!

moderation commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Sep 8, 2023

Uh oh!

jbohanon commented Sep 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbohanon commented Sep 9, 2023

Uh oh!

jmarantz commented Sep 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarantz commented Sep 7, 2023 •

edited

Loading

moderation commented Sep 8, 2023 •

edited

Loading

moderation commented Sep 8, 2023 •

edited

Loading

jbohanon commented Sep 9, 2023 •

edited

Loading