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

perf: reducing the lock strength of the soa cache entry #2285

Merged
merged 4 commits into from
Oct 3, 2024

Conversation

djx30103
Copy link
Contributor

When a large number of applications are made, there will be a large number of lock seizures and lower performance

djx30103 and others added 2 commits September 29, 2024 17:57
Utilize the singleflight package to ensure concurrent DNS SOA lookups are performed only once per unique request. This change improves efficiency by preventing multiple redundant lookups for the same fully qualified domain name (FQDN).
@ldez ldez self-requested a review September 29, 2024 11:58
@ldez
Copy link
Member

ldez commented Sep 29, 2024

Hello,

singleflight.Group is a map with a lock and sync.Map is also a map with a lock.
So you replaced a map + a lock with 2 map + 2 locks.

Also:

// The Map type is optimized for two common use cases: (1) when the entry for a given
// key is only ever written once but read many times, as in caches that only grow,
// or (2) when multiple goroutines read, write, and overwrite entries for disjoint
// sets of keys. In these two cases, use of a Map may significantly reduce lock
// contention compared to a Go map paired with a separate [Mutex] or [RWMutex].
https://github.com/golang/go/blob/2bffb8b3fb2d9137ccfa87fc35137371b86a2e96/src/sync/map.go#L19-L23

The usage inside lego is neither case 1 (because the entry for a given is written several times) nor case 2 (because it's not a disjoint set of keys).

I think there are no advantages.

Additionally, with your changes, the ClearFqdnCache function is no longer concurrently safe.

Also, I need to understand why you are doing this PR:

  • I need proof that there is a problem
  • I also need proof that your PR is fixing something.

@ldez
Copy link
Member

ldez commented Sep 30, 2024

Your commit is not the expected answer, I'm waiting for a comment (in English).

Otherwise, are you an employee of @fit2cloud?

@djx30103
Copy link
Contributor Author

djx30103 commented Sep 30, 2024

Hello,
I am not a fit2cloud employee.

The lines of code may not match, I have modified some of the handling of defaultNameservers, but the logic of lookupNameservers I haven't changed.

image
image

This is before modification, 200 concurrent.

image

This is after the modification, 200 concurrent.

image

Blocking changed from lock to udp, and each blocks its own udp request, so the result will be returned quickly, reducing the number of coroutines

@ldez ldez changed the title perf: Reducing the lock strength of the soa cache entry perf: reducing the lock strength of the soa cache entry Sep 30, 2024
@ldez ldez added this to the v4.19 milestone Oct 2, 2024
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Based on the information from this PR:

  • 2 different users on the same PR
  • 200 concurrent requests per second

I can be wrong, but I think there is a company behind this, and it's not a problem.

But, please consider asking your company to donate.
If there is no company behind this, please consider donating anyway.
This will be appreciated, thank you ❤️

https://github.com/sponsors/ldez

@ldez ldez merged commit a83482c into go-acme:master Oct 3, 2024
4 checks passed
muFqdnSoaCache.Lock()
fqdnSoaCache = map[string]*soaCacheEntry{}
muFqdnSoaCache.Unlock()
fqdnSoaCache.Clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

Clear() was added in Go 1.23, please update go.mod or use API compatible with Go 1.22. Thank you

Copy link
Member

Choose a reason for hiding this comment

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

We will not update to go1.23, but I will change this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants