Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fqdn: avoid expensive sort/unique of names during GC #30920

Merged
merged 1 commit into from Mar 15, 2024
Merged

fqdn: avoid expensive sort/unique of names during GC #30920

merged 1 commit into from Mar 15, 2024

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Feb 22, 2024 •
edited

Return the names affected by GC in a set instead of a slice. This avoids having to repeatedly sort and unique the slice which is a potentially expensive operation if there are many FQDN names to garbage collect. The consumer in (*NameManager).GC will expect a sets.Set already anyway.

@tklauser tklauser added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. dont-merge/preview-only Only for preview or testing, don't merge it. area/fqdn Affects the FQDN policies feature labels Feb 22, 2024
@tklauser
Copy link
Member Author

/test

@tklauser tklauser changed the title fqdn: return names affected by GC in set instead of slice to avoid expensive sort fqdn: avoid expensive sort/unique of names during GC Mar 13, 2024
@tklauser
fqdn: avoid expensive sort/unique of names during GC
16caf0e 
Return the names affected by GC in a set instead of a slice. This avoids
having to repeatedly sort and unique the slice which is a potentially
expensive operation if there are many FQDN names to garbage collect. The
consumer in `(*NameManager).GC` will expect a `sets.Set` already anyway.

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser
Copy link
Member Author

/test

@tklauser tklauser marked this pull request as ready for review March 13, 2024 14:23
@tklauser tklauser requested a review from a team as a code owner March 13, 2024 14:23
@tklauser tklauser requested a review from pippolo84 March 13, 2024 14:23
@tklauser tklauser removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Mar 13, 2024
@tklauser tklauser enabled auto-merge March 13, 2024 16:11
pippolo84
pippolo84 approved these changes Mar 15, 2024
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM!

Looking at cleanupExpiredEntries and cleanupOverLimitEntries I wonder if we should try a different data structure to store the entries in the forward map and keep them sorted by expiration time (maybe something like an indexed priority queue?)

@tklauser tklauser added this pull request to the merge queue Mar 15, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 15, 2024
Merged via the queue into main with commit 069d11a Mar 15, 2024
225 checks passed
@tklauser tklauser deleted the pr/tklauser/fqdn-gc-sets branch March 15, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pippolo84 pippolo84 pippolo84 approved these changes

Assignees
No one assigned
Labels
area/fqdn Affects the FQDN policies feature ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tklauser @pippolo84