performance: optimized string and various other XDS improvements #1016
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
istio-testing
added
do-not-merge/work-in-progress
howardjohn
changed the title
istio-testing
removed
the
do-not-merge/work-in-progress
istio-testing
added
size/XXL
that's more than I thought we'd save, nice. |
bleggett
approved these changes
May 3, 2024
| /// * Immutable | ||
| /// This is mostly provided by a library, ArcStr, we just provide a very thin wrapper around it | ||
| /// for some flexibility. | ||
| pub type Strng = ArcStr; |
There was a problem hiding this comment.
Strictly speaking we don't need this type alias, we could just have ArcStr + RichStrng(ArcStr)?
That's a little more obvious and it's also easy to look at a type named ArcStr and know what it does.
But that's mostly a stylistic nit I guess, and if we ever want to use a different ArcStr impl, this insulates from that a bit.
There was a problem hiding this comment.
I was on the fence about the Strng thing. It was more useful when doing the interning library since their type name was unwieldy
bleggett
added
the
cherrypick/release-1.22
howardjohn
force-pushed
the
perf/intern-xds
branch
from
4c11a76
to
9a55c79
Compare
|
In response to a cherrypick label: #1016 failed to apply on top of branch "release-1.22": |
|
In response to a cherrypick label: new issue created for failed cherrypick: #1020 |
howardjohn
added a commit
to howardjohn/ztunnel
that referenced
this pull request
May 3, 2024
…io#1016) * Initial * wip * prod compiling * bump max * wip diagnostics * performance: avoid clone of workload on conversion * performance: avoid allocating vector for all resources * intern: xds types * performance: optimize endpoint_uid * perf: do not allocate endpoints vector * performance: do not clone services map * use arcstr instead * metrics back on * Tests compile * format * fix and format * Move rbac over too * Move more over * rebase (cherry picked from commit 5eb8906)
istio-testing
pushed a commit
that referenced
this pull request
May 4, 2024
* logs: fix dst.namespace and missing dst.service (#1017) (cherry picked from commit 543a45b) * misc: remove Default identity from prod codepath (#1018) We didn't use it anywhere which is good -- but it was a risk (cherry picked from commit a84ce37) * performance: optimized string and various other XDS improvements (#1016) * Initial * wip * prod compiling * bump max * wip diagnostics * performance: avoid clone of workload on conversion * performance: avoid allocating vector for all resources * intern: xds types * performance: optimize endpoint_uid * perf: do not allocate endpoints vector * performance: do not clone services map * use arcstr instead * metrics back on * Tests compile * format * fix and format * Move rbac over too * Move more over * rebase (cherry picked from commit 5eb8906) * add missing local example (#1014) * add missing local example * lint * fmt (cherry picked from commit 3c7fc71) * logging: disable regex (#987) (cherry picked from commit c68413be4387680372df53fb496b50132fb6fef3) (cherry picked from commit 4605fdb) --------- Co-authored-by: Yuval Kohavi <yuval.kohavi@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #988
This PR introduces a new string representation. See issue above for some more background.
I initially implemented this using
internment::ArcIntern<str>. This showed improvements over master, but I compared to using ArcStr and found actually better memory utilization with ArcStr, and much less complexity and risk. Conveniently, due to thestrngmodule abstracting things, the swap was pretty much trivial (just a few small changes due to different conversion helpers in the libraries). So if we do want to switch things in the future, it shouldn't be too bad.This optimizes the XDS path (and, presumably, others -- but I only measured XDS much).
My test covered 30k pods with 2k services. I loaded this up and checked memory usage (total and peak).
Before: 1.8GB total, 700mb retained
After: 1.1GB total, 520mb retained
Roughly 100mb of improvement came from switching the string representation, and 80mb came from other optimizations. Not confident in that breakdown as I was going back and forth moving things over + optimizing.
All optimizations here are guided by profiling. End profile (github
Not much low hanging fruit left, most of the memory is in our hashmaps.
Optimizations aside from string changes: