selectorcache: test new index in the selector cache id -> selectors#43376
Closed
odinuge wants to merge 5 commits intocilium:mainfrom
Closed
selectorcache: test new index in the selector cache id -> selectors#43376odinuge wants to merge 5 commits intocilium:mainfrom
odinuge wants to merge 5 commits intocilium:mainfrom
Conversation
7e8ff46 to
5b76c61
Compare
odinuge
commented
Dec 17, 2025
| if namespace != "" { | ||
| for rKey := range p.rulesByNamespace[namespace] { | ||
|
|
||
| if subjectIsNode := securityIdentity.ID == identity.ReservedIdentityHost; subjectIsNode { |
Member
Author
There was a problem hiding this comment.
Based on #42580, I think with that in place we will always get the updated node identity in that selector cache - so I'm pretty sure we in that situation are fine to just always rely on the index inside the selector cache without looping through all the rules.
odinuge
commented
Dec 17, 2025
| // iterating selectors requires read lock | ||
| for key, sel := range sc.selectors.All() { | ||
| selections := sel.GetSelectionsAt(version) | ||
| selections := sel.GetSortedSelectionsAt(version) |
Member
Author
There was a problem hiding this comment.
This probably has no need to get the sorted version, and a set is fine.
This introduces a separate sorted structure that can be used by the components that need it. Previously all selections were sorted. This very costly in the hotpath of the selection updates, as sorting 10k+ slices is pretty expensive. This still creates a copy of the selections, but it does not sort it. This can in theory result in higher memory usage since we now keep a set (in practice a map), and the sorted slice if asked for. In practice not many things need the sorted version, notably the dnsproxy as well as some l7/xds configs. Its also not totally sure that those always need it either - and in the cases they need it, they also end up keeping it in memory - so the diff is pretty small in practice. Most of the datapath is also using updates, so its not relying on the sorted version all the time. Its a bit hard to benchmark this directly without cherry-picking results, but a simple test-case where a single selector selects say 20k identities (eg. the 'cluster' entity), that are updated say 5 times/sec - results in a lot of cpu cycles spent on sorting that slice. Signed-off-by: Odin Ugedal <odin@ugedal.com> Signed-off-by: Odin Ugedal <ougedal@palantir.com>
Signed-off-by: Odin Ugedal <ougedal@palantir.com>
Signed-off-by: Odin Ugedal <odin@ugedal.com> Signed-off-by: Odin Ugedal <ougedal@palantir.com>
This swaps the set.Set to a part.Set, so that we can avoid cloning the whole datastructure on each write. This does result in more allocations in general, but overall it looks like a win since we reduce the time it takes to clone it. The same applies to the total amount of allocated bytes, that seems to go down drastically. Signed-off-by: Odin Ugedal <odin@ugedal.com> Signed-off-by: Odin Ugedal <ougedal@palantir.com>
This is still TBD waiting on the split between peer and subject selector caches, but will be useful to avoid having to loop through all the rules on every single regeneration. If there are say 10k+ policy rules but only 100 IDs, this is pretty cheap compared to looping through all of them. This is still proof-of-concept and will require more benchmarking to figure out if its workable or not. Signed-off-by: Odin Ugedal <odin@ugedal.com> Signed-off-by: Odin Ugedal <ougedal@palantir.com>
Member
Author
|
Testing this using the benchmarks in #43407 yields the following results; |
5b76c61 to
9be5a28
Compare
Member
Author
|
/test |
8 tasks
|
This pull request has been automatically marked as stale because it |
|
This pull request has not seen any activity since it was marked stale. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This is still TBD waiting on the split between peer and subject selector
caches, but will be useful to avoid having to loop through all the
rules on every single regeneration. If there are say 10k+ policy rules
but only 100 IDs, this is pretty cheap compared to looping through all
of them.
This is still proof-of-concept and will require more benchmarking to
figure out if its workable or not.
Built on top of #43368, and waiting for #42580.
Fixes: #issue-number