identity/cache: only call SortedList for release #27796
Merged
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
bimmlerd
added
kind/performance
|
/test (was completely green, rerunning for second commit) |
bimmlerd
force-pushed
the
pr/bimmlerd/avoid-sorted-list-calls
branch
from
7e73fff
to
174bfe9
Compare
bimmlerd
force-pushed
the
pr/bimmlerd/avoid-sorted-list-calls
branch
from
174bfe9
to
f5a8e36
Compare
|
/test |
aanm
approved these changes
Aug 30, 2023
old is |
bimmlerd
force-pushed
the
pr/bimmlerd/avoid-sorted-list-calls
branch
from
f5a8e36
to
880333c
Compare
This case is exercised heavily in the toFQDN policy incremental policy computation flow. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
bimmlerd
force-pushed
the
pr/bimmlerd/avoid-sorted-list-calls
branch
from
880333c
to
75cf55b
Compare
|
/test |
aditighag
approved these changes
Aug 31, 2023
schlosna
reviewed
Sep 1, 2023
This is on the hot path when we have a fqdn policy for S3, where a single hostname maps to many IPs. This occurs due to a combination of factors: 1. We allocate a CIDR identitiy for each IP for a hostname which matches a fqdn policy. 2. For each CIDR identity, we generate labels for all super-CIDRs (i.e. CIDRs which contain this CIDR). 3. We opportunistically allocate an identity for all IPs which are matched by a fqdn selector. For those we already knew, other parts of the code decrement the ref count again. 4. We use the SortedList serialization as the key to lookup for existing identities here, which sorts and serializes the labels to a byte array, which is reasonably expensive. Since we can't as easily fix the other factors at play here, at least avoid doing it twice for each label set during the opportunistic acquire/release path. We can lookup by identity for the fast path, and build the string representation for the slower path if need be. We hit this path for every IP returned by S3 that's still being kept alive (be that because of DNS TTL or the zombie mechanism), hence we can easily get to 5k acquire/release pairs per DNS request. While we're at it, also reduce the critical section by moving the SortedList call outside in lookupOrCreate. Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
SortedList appears prominently in both CPU and Heap pprofs when in a
scenario with an FQDN policy matching S3. This is because DNS requests
to S3 return short-lived DNS responses from a pool of many IPs, each of
which will receive a CIDR identitiy in cilium.
Since we furthermore call SortedList repeatedly for these CIDR
identities (represented as a set of CIDR labels containing all
super-CIDRs), avoiding ~32 buffer allocations per call is worth it.
Before:
Labels_SortedList-10 1000000 3079 ns/op 504 B/op 13 allocs/op
Labels_SortedListCIDRIDs-10 52702 21417 ns/op 3680 B/op 41 allocs/op
After:
Labels_SortedList-10 1000000 2164 ns/op 360 B/op 3 allocs/op
Labels_SortedListCIDRIDs-10 72180 15444 ns/op 1624 B/op 3 allocs/op
Benchstat:
│ old │ opt │
│ sec/op │ sec/op vs base │
Labels_SortedList-10 3.279µ ± 6% 2.209µ ± 6% -32.65% (p=0.000 n=10)
│ B/op │ B/op vs base │
Labels_SortedList-10 504.0 ± 0% 360.0 ± 0% -28.57% (p=0.000 n=10)
│ allocs/op │ allocs/op vs base │
Labels_SortedList-10 13.000 ± 0% 3.000 ± 0% -76.92% (p=0.000 n=10)
pkg: github.com/cilium/cilium/pkg/labels/cidr
│ old │ opt │
│ sec/op │ sec/op vs base │
Labels_SortedListCIDRIDs-10 21.23µ ± 5% 14.89µ ± 9% -29.87% (p=0.000 n=10)
│ B/op │ B/op vs base │
Labels_SortedListCIDRIDs-10 3.594Ki ± 0% 1.586Ki ± 0% -55.87% (p=0.000 n=10)
│ allocs/op │ allocs/op vs base │
Labels_SortedListCIDRIDs-10 41.000 ± 0% 3.000 ± 0% -92.68% (p=0.000 n=10)
Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
bimmlerd
force-pushed
the
pr/bimmlerd/avoid-sorted-list-calls
branch
from
75cf55b
to
be37350
Compare
|
I've pushed the above-suggested change to pull the call to |
|
/test |
|
CI triage:
|
|
/ci-runtime |
aanm
approved these changes
Sep 6, 2023
joamaki
approved these changes
Sep 7, 2023
aditighag
approved these changes
Sep 7, 2023
maintainer-s-little-helper
bot
added
the
ready-to-merge
aditighag
reviewed
Sep 7, 2023
julianwiedmann
added
backport-done/1.12
squeed
added
the
needs-backport/1.14
tklauser
added
backport-pending/1.14
github-actions
bot
added
backport-done/1.14
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.
First commit introduces a specific benchmark, the next two commits are optimizations. Commit msgs should be descriptive. The first optimization commit does not improve benchmark time as the benchmark doesn't exercise that path, but the second optimization clearly improves benchmark time.
Below the metrics of a kind cluster which runs 10 pods which send a single UDP packet to
host1.test.org., while having a custom DNS server which responds with 5 random IPs forhost1.test.org.with a TTL of 5 seconds. This mimicks S3 behaviour.In the graph, you can see that there's a doubling of identities, that's the identity restoration grace period of 10 minutes; it shows when the pods were restarted with the two commits of this PR applied. They show an approximate reduction of 5MiB/s and 50KHz allocations.