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.

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

FQDN: pick some low-hanging performance fruit #29691

Merged
merged 4 commits into from
Dec 12, 2023

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Dec 7, 2023

This fixes some low-hanging-fruit, performance-wise, of the FQDN subsystem. It introduces a benchmark that stresses NameManager.UpdateGeneratedDNS(), which is in the critical path (and behind a lock!) for returning DNS responses.

The changes are:

  1. Use sets.Set for determining IP uniqueness. This allocates a bit more memory, but has the advantage of many fewer allocations
  2. Don't sort unnecessarily. We were doing a lot of unnecessary sorts in the DNSCache. These are only needed for tests, so just axe 'em.
  3. Don't build an expensive logrus.Fields object just to skip at non-Debug level.

Benchmark results:

                     │ bench/00.out │            bench/01.out            │
                     │    sec/op    │   sec/op    vs base                │
UpdateGenerateDNS-12     3.558 ± 1%   2.157 ± 1%  -39.37% (p=0.000 n=10)

                     │ bench/00.out │             bench/01.out             │
                     │     B/op     │     B/op      vs base                │
UpdateGenerateDNS-12   902.9Mi ± 0%   532.1Mi ± 0%  -41.06% (p=0.000 n=10)

                     │ bench/00.out │            bench/01.out             │
                     │  allocs/op   │  allocs/op   vs base                │
UpdateGenerateDNS-12    9.034M ± 0%   1.009M ± 0%  -88.83% (p=0.000 n=10)
Optimize IP/FQDN management in the DNSCache

@squeed squeed requested review from a team as code owners December 7, 2023 09:53
@squeed squeed requested a review from danehans December 7, 2023 09:53
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 7, 2023
@squeed squeed added kind/performance There is a performance impact of this. release-note/misc This PR makes changes that have no direct user impact. area/fqdn Affects the FQDN policies feature labels Dec 7, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 7, 2023
This is a simple benchmark that determine's the NameManager's
contribution to the FQDN response flow.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
We were doing needless sorting and searching to determine uniqueness.
Only determine unique IDs when necessary. And, switch to using sets.Set
for uniqueness. It uses more memory, but a lot less CPU and fewer allocations.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
This code is on the critical path for DNS responses. By not sorting, we
can save up to 50% of time spent in the NameManager.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
Looking at CPU and memory profiling, this throwaway debug message was
taking up to 50% of memory and a good chunk of CPU. Don't even bother
allocating the logrus.Fields object unless we know we'll log.

Signed-off-by: Casey Callendrello <cdc@isovalent.com>
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Aswesome, thanks! I'm a low confidence reviewer, but this looks good to me overall. Only one thing which is unclear to me.

I would also be curious to see benchmarks for different data set sizes. While I expect a hashmap to be more efficient for large-ish sets of IPs, I would also expect that if there is only a hand full IPs that the old slice+sort approach might be faster.

pkg/fqdn/cache.go Show resolved Hide resolved
pkg/fqdn/cache.go Show resolved Hide resolved
pkg/fqdn/helpers.go Show resolved Hide resolved
pkg/fqdn/cache.go Show resolved Hide resolved
@joestringer
Copy link
Member

/test

@joestringer joestringer added this pull request to the merge queue Dec 12, 2023
Merged via the queue into cilium:main with commit 8e14481 Dec 12, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/fqdn Affects the FQDN policies feature kind/performance There is a performance impact of this. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants