json: move all implemention methods of Streamer to .h

[wbpcode]

Commit Message: json: move all implemention methods of Streamer to .h Additional Description:

Part of #35823. I split this out to simplify the review.

This PR:

1. move the methods' body to .h

Risk Level: low. Testing: n/a. Docs Changes: n/a. Release Notes: n/a. Platform Specific Features: n/a.

[wbpcode]

/assign-from @envoyproxy/senior-maintainers

[repokitteh-read-only[bot]]

@envoyproxy/senior-maintainers assignee is @RyanTheOptimist

:cat:

Caused by: a https://github.com/envoyproxy/envoy/pull/35891#issuecomment-2316593453 was created by @wbpcode.

see: more, trace.

[jmarantz]

Yes. When you are doing a massive code move like this I'd prefer that no other changes at all be made. Just move the code and drop the .cc file but don't change anything else.

Then we can do follow-up incremental improvements to the code and discuss those. Now I have no hope of finding them.

[wbpcode]

cc @jmarantz done.

code move only now.

[jmarantz]

Looks great. Can you add the admin stats benchmark before/after for this PR? That will establish a baseline for other changes you might want to make (e.g. using visit pattern) as well as assure as that this is the same or better.

[wbpcode]

(e.g. using visit pattern) as well as assure as that this is the same or better.

I have tried and it make performance a little worse.

[wbpcode]

Previous:

BM_AllCountersJson/per_endpoint_stats_disabled_mean               592 ms          592 ms           10 output per iteration: 135789011
BM_AllCountersJson/per_endpoint_stats_enabled_mean                967 ms          967 ms           10 output per iteration: 180082411
BM_UsedCountersJson/per_endpoint_stats_disabled_mean             13.2 ms         13.2 ms           10 output per iteration: 1348901
BM_UsedCountersJson/per_endpoint_stats_enabled_mean               421 ms          421 ms           10 output per iteration: 45642301
BM_FilteredCountersJson/per_endpoint_stats_disabled_mean          176 ms          176 ms           10 output per iteration: 12
BM_FilteredCountersJson/per_endpoint_stats_enabled_mean           541 ms          540 ms           10 output per iteration: 12
BM_HistogramsJson/per_endpoint_stats_disabled_mean               10.3 ms         10.3 ms           10 output per iteration: 1820890
BM_HistogramsJson/per_endpoint_stats_enabled_mean                10.5 ms         10.5 ms           10 output per iteration: 1820890

Inlined version:

BM_AllCountersJson/per_endpoint_stats_disabled_mean               614 ms          612 ms           10 output per iteration: 135789011
BM_AllCountersJson/per_endpoint_stats_enabled_mean                978 ms          978 ms           10 output per iteration: 180082411
BM_UsedCountersJson/per_endpoint_stats_disabled_mean             13.6 ms         13.6 ms           10 output per iteration: 1348901
BM_UsedCountersJson/per_endpoint_stats_enabled_mean               415 ms          415 ms           10 output per iteration: 45642301
BM_FilteredCountersJson/per_endpoint_stats_disabled_mean          175 ms          175 ms           10 output per iteration: 12
BM_FilteredCountersJson/per_endpoint_stats_enabled_mean           504 ms          504 ms           10 output per iteration: 12
BM_HistogramsJson/per_endpoint_stats_disabled_mean               10.4 ms         10.4 ms           10 output per iteration: 1820890
BM_HistogramsJson/per_endpoint_stats_enabled_mean                10.4 ms         10.4 ms           10 output per iteration: 1820890

Some a little better, some a little worse. Mainly no difference.

cc @jmarantz here.

[jmarantz]

THe benchmark is the inlined version? Can you show it against the current version?

[wbpcode]

THe benchmark is the inlined version? Can you show it against the current version?

@jmarantz updated.

[repokitteh-read-only[bot]]

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh). envoyproxy/coverage-shephards assignee is @alyssawilk

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/35891 was synchronize by wbpcode.

see: more, trace.

[wbpcode]

Was your machine totally idle during both tests? Or do you think there's a perf improvement in your template version?

/wait

Yeah, I am sure it's basically idle except the background Vscode and IM App. And I didn't use them when I running the test. (The env is complete same with the templated version.)

perf degradation seems minor

I think this PR doesn't bring degration? Although some case it's a little worse, but also it is a little better in some other cases. And the fluctuation ratio is very minor.

but I thought in the template version things got better.

It also is very very minor improvement. I think we can completely ignore it.

We should care the code logic rather than the uncertain behavior of compiler.

The compiler's low level behavior will changes with version updating. The very minor improvement or degration from that is also uncertain and should be a basis when we making decision.

This PR (and templated one) doesn't change any logic from C++ perspective. And the performance no big change. Then, I think it is enough.

[wbpcode]

Looks like you still have CI issues; ping me and I'll reapprove when that's sorted.

rbac is flaky. Help it's fine this time.

[wbpcode]

/retest

[phlax]

re rbac flake see https://github.com/envoyproxy/envoy/pull/35899 and related ticket

[jmarantz]

all good from my end.

Happy to review other incremental changes to improve the algo if you have them, as long as it's easy to see them in the tool. The change to templating should also be relatively incremental at this point.