improve substitution formatter parsing performance #35498

wbpcode · 2024-07-30T11:53:09Z

Commit Message: improve substitution formatter parsing performance
Additional Description:

Previous parsing performance result:

---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
BM_AccessLogFormatterSetup            75744 ns        75605 ns         9401

Latest parsing performance result:

---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
BM_AccessLogFormatterSetup             8932 ns         8917 ns        78153

Risk Level: mid. Core code change.
Testing: unit.
Docs Changes: n/a.
Release Notes: no behavior change.
Platform Specific Features: n/a.

Signed-off-by: wbpcode <[email protected]>

…ptimize-date-formatter

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2024-07-30T11:56:32Z

By the way, from the performance results, the json formatter is too slow...

---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
BM_AccessLogFormatterSetup             8932 ns         8917 ns        78153
BM_AccessLogFormatter                   510 ns          505 ns      1396258
BM_StructAccessLogFormatter            7153 ns         7140 ns       103466
BM_TypedStructAccessLogFormatter       5390 ns         5384 ns       113175
BM_JsonAccessLogFormatter             12472 ns        12457 ns        52666
BM_TypedJsonAccessLogFormatter        10999 ns        10985 ns        61242
BM_FormatterCommandParsing             40.1 ns         40.0 ns     17475447

ravenblackx

Big fan of const std::string& to string_view migration.

Arguably the date formatting change could be a separate PR to the string-parameter change, but I'm okay with approving both together.

ravenblackx · 2024-07-30T14:16:34Z

test/common/formatter/substitution_formatter_speed_test.cc

+  testing::NiceMock<MockTimeSystem> time_system;
+  ON_CALL(time_system, systemTime())
+      .WillByDefault(testing::Return(SystemTime(std::chrono::seconds(1234567890))));
+  ON_CALL(time_system, monotonicTime())
+      .WillByDefault(testing::Return(MonotonicTime(std::chrono::seconds(1234567890))));
+


Since this block is repeated a lot, probably worth a helper function so you can just do MockTimeSystem time_system = makeMockTime() (the function can still wrap it in a NiceMock but you won't have to say it's so every time, or set up the ON_CALLs)

Good idea, let's have a try.

wbpcode · 2024-07-30T14:37:22Z

cc @ravenblackx Thanks so much for the approval. And yeah, actually DateFormatter refactoring is a separated PR. See #35450

I developed this patch based on #35450. And I will do a rebase after #35450 is landed.

Signed-off-by: wbpcode <[email protected]>

ravenblackx · 2024-07-30T15:52:09Z

source/extensions/formatter/metadata/metadata.h

@@ -16,17 +16,9 @@ namespace Formatter {
 class MetadataFormatterCommandParser : public ::Envoy::Formatter::CommandParser {
 public:
  MetadataFormatterCommandParser();


Constructor should be =default or not declared at all now that the impl was removed.

Hmmmmm, so weird. I need to re run the tests in my local env.

I remember I had fixed all these. Seems some code changes are lost.

Signed-off-by: wbpcode <[email protected]>

kyessenov · 2024-07-30T17:31:22Z

By the way, from the performance results, the json formatter is too slow...

---------------------------------------------------------------------------
Benchmark                                 Time             CPU   Iterations
---------------------------------------------------------------------------
BM_AccessLogFormatterSetup             8932 ns         8917 ns        78153
BM_AccessLogFormatter                   510 ns          505 ns      1396258
BM_StructAccessLogFormatter            7153 ns         7140 ns       103466
BM_TypedStructAccessLogFormatter       5390 ns         5384 ns       113175
BM_JsonAccessLogFormatter             12472 ns        12457 ns        52666
BM_TypedJsonAccessLogFormatter        10999 ns        10985 ns        61242
BM_FormatterCommandParsing             40.1 ns         40.0 ns     17475447

Any idea why JSON is 20x slower? Is it because it's using some library?

wbpcode · 2024-07-31T02:41:32Z

Any idea why JSON is 20x slower? Is it because it's using some library?

Not sure. But format the output proto struct has taken 10x time than the text formatter.

ravenblackx · 2024-07-31T05:48:23Z

/retest

…efactor-formatter-with-re2

Signed-off-by: wbpcode <[email protected]>

wbpcode · 2024-08-01T02:42:36Z

#35450 was landed. Now, it's this PR's turn.

wbpcode · 2024-08-01T11:01:48Z

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2024-08-01T11:01:53Z

@envoyproxy/senior-maintainers assignee is @mattklein123

🐱

Caused by: a #35498 (comment) was created by @wbpcode.

see: more, trace.

…efactor-formatter-with-re2

wbpcode · 2024-08-02T16:40:41Z

/retest

Commit Message: improve substitution formatter parsing performance Additional Description: See envoyproxy#35466 Previous parsing performance result: ``` --------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------- BM_AccessLogFormatterSetup 75744 ns 75605 ns 9401 ``` Latest parsing performance result: ``` --------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------- BM_AccessLogFormatterSetup 8932 ns 8917 ns 78153 ``` Risk Level: mid. Core code change. Testing: unit. Docs Changes: n/a. Release Notes: no behavior change. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode <[email protected]> Signed-off-by: Martin Duke <[email protected]>

Commit Message: improve substitution formatter parsing performance Additional Description: See envoyproxy#35466 Previous parsing performance result: ``` --------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------- BM_AccessLogFormatterSetup 75744 ns 75605 ns 9401 ``` Latest parsing performance result: ``` --------------------------------------------------------------------------- Benchmark Time CPU Iterations --------------------------------------------------------------------------- BM_AccessLogFormatterSetup 8932 ns 8917 ns 78153 ``` Risk Level: mid. Core code change. Testing: unit. Docs Changes: n/a. Release Notes: no behavior change. Platform Specific Features: n/a. --------- Signed-off-by: wbpcode <[email protected]> Signed-off-by: asingh-g <[email protected]>

wbpcode added 6 commits July 26, 2024 21:11

date formatter: fix the precision of date formatter

607e485

Signed-off-by: wbpcode <[email protected]>

make gcc happy

19b1a4f

Signed-off-by: wbpcode <[email protected]>

add release note

13b1c3f

Signed-off-by: wbpcode <[email protected]>

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-o…

f995c15

…ptimize-date-formatter

improve substitution formatter parsing performance

c015747

Signed-off-by: wbpcode <[email protected]>

fix other extensions and test

b868d9e

Signed-off-by: wbpcode <[email protected]>

wbpcode requested review from alyssawilk, kyessenov, ravenblackx and nezdolik as code owners July 30, 2024 11:53

ravenblackx previously approved these changes Jul 30, 2024

View reviewed changes

ravenblackx self-assigned this Jul 30, 2024

fix ci and address comment

28035ec

Signed-off-by: wbpcode <[email protected]>

wbpcode dismissed ravenblackx’s stale review via 28035ec July 30, 2024 14:48

wbpcode requested a review from soulxu as a code owner July 30, 2024 14:48

minor update

89fec6d

Signed-off-by: wbpcode <[email protected]>

ravenblackx reviewed Jul 30, 2024

View reviewed changes

minor update

2b628f9

Signed-off-by: wbpcode <[email protected]>

ravenblackx previously approved these changes Jul 31, 2024

View reviewed changes

wbpcode added 2 commits August 1, 2024 09:55

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-r…

b06acdd

…efactor-formatter-with-re2

fix conflict

e05e3eb

Signed-off-by: wbpcode <[email protected]>

wbpcode dismissed ravenblackx’s stale review via e05e3eb August 1, 2024 02:15

repokitteh-read-only bot assigned mattklein123 Aug 1, 2024

ravenblackx previously approved these changes Aug 1, 2024

View reviewed changes

mattklein123 previously approved these changes Aug 2, 2024

View reviewed changes

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-r…

a5a1e6c

…efactor-formatter-with-re2

wbpcode dismissed stale reviews from mattklein123 and ravenblackx via a5a1e6c August 2, 2024 14:40

ravenblackx approved these changes Aug 2, 2024

View reviewed changes

wbpcode enabled auto-merge (squash) August 2, 2024 16:40

wbpcode merged commit 6c645a1 into envoyproxy:main Aug 2, 2024
51 checks passed

wbpcode deleted the dev-refactor-formatter-with-re2 branch August 2, 2024 20:45

wbpcode linked an issue Aug 29, 2024 that may be closed by this pull request

use re2 for the substitution formatter parsing and prefer the string view #35466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve substitution formatter parsing performance #35498

improve substitution formatter parsing performance #35498

wbpcode commented Jul 30, 2024 •

edited

Loading

wbpcode commented Jul 30, 2024

ravenblackx left a comment

ravenblackx Jul 30, 2024

wbpcode Jul 30, 2024 •

edited

Loading

wbpcode commented Jul 30, 2024

ravenblackx Jul 30, 2024

wbpcode Jul 30, 2024

kyessenov commented Jul 30, 2024

wbpcode commented Jul 31, 2024

ravenblackx commented Jul 31, 2024

wbpcode commented Aug 1, 2024

wbpcode commented Aug 1, 2024

repokitteh-read-only bot commented Aug 1, 2024

wbpcode commented Aug 2, 2024

improve substitution formatter parsing performance #35498

improve substitution formatter parsing performance #35498

Conversation

wbpcode commented Jul 30, 2024 • edited Loading

wbpcode commented Jul 30, 2024

ravenblackx left a comment

Choose a reason for hiding this comment

ravenblackx Jul 30, 2024

Choose a reason for hiding this comment

wbpcode Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Jul 30, 2024

ravenblackx Jul 30, 2024

Choose a reason for hiding this comment

wbpcode Jul 30, 2024

Choose a reason for hiding this comment

kyessenov commented Jul 30, 2024

wbpcode commented Jul 31, 2024

ravenblackx commented Jul 31, 2024

wbpcode commented Aug 1, 2024

wbpcode commented Aug 1, 2024

repokitteh-read-only bot commented Aug 1, 2024

wbpcode commented Aug 2, 2024

wbpcode commented Jul 30, 2024 •

edited

Loading

wbpcode Jul 30, 2024 •

edited

Loading