-
Notifications
You must be signed in to change notification settings - Fork 370
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
Lazily initialize localPodInformer in a more readable way #5697
Conversation
/test-all |
cmd/antrea-agent/agent.go
Outdated
if o.enableNodePortLocal || enableBridgingMode || enableMulticlusterNP || enableFlowExporter || | ||
features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) || | ||
features.DefaultFeatureGate.Enabled(features.TrafficControl) { | ||
localPodInformer := lazy.New[cache.SharedIndexInformer](func() cache.SharedIndexInformer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be equivalent to use sync.OnceValue
?
localPodInformerGet := sync.OnceValue(func() cache.SharedIndexInformer {
// ...
return coreinformers.NewFilteredPodInformer(...)
})
// ...
localPodInformerGet()
localPodInformerGet()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion, I didn't know this function but there are still two reasons why I feel the current implementation is more suitable:
- It's more readable when we need to pass the "getter" to another function like why I need the interface:
https://github.com/antrea-io/antrea/blob/6831d5701c0bfeb1002c69d88ae619773f48e6f4/pkg/agent/controller/networkpolicy/networkpolicy_controller.go#L148C2-L148C13 - It takes less code on caller side to know whether it's ever evaluated. Otherwise we will have to have another local varible to track it for latter running it.
cmd/antrea-agent/agent.go
Outdated
if o.enableNodePortLocal || enableBridgingMode || enableMulticlusterNP || enableFlowExporter || | ||
features.DefaultFeatureGate.Enabled(features.SecondaryNetwork) || | ||
features.DefaultFeatureGate.Enabled(features.TrafficControl) { | ||
localPodInformer := lazy.New[cache.SharedIndexInformer](func() cache.SharedIndexInformer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: is it required to specify the type argument here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed the type argument, thanks.
af1e32d
to
811c456
Compare
Instead of checking various conditions to determine whether localPodInformer should be initialized, which is error-prone and looks ugly, this patch adds a Generic interface for lazy evaluation and uses it to initialize localPodInformer when it's necessary. Signed-off-by: Quan Tian <[email protected]>
811c456
to
2a0c329
Compare
getter func() T | ||
// res is the cached result. | ||
res T | ||
done uint32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be missing something, but since we are using a mutex anyway, it's unclear to me what the benefit of using atomic
functions is, compared to using the mutex "everywhere":
func (l *lazy[T]) Get() T {
l.m.Lock()
defer l.m.Unlock()
if !l.done {
l.res = l.getter()
l.done = true
}
return l.res
}
func (l *lazy[T]) Evaluated() bool {
l.m.Lock()
defer l.m.Unlock()
return l.done
}
The main difference is is in the behavior of Evaluated
if the object is currently being evaluated (Evaluated
will now block and return true). I don't know if one approach is more desired here.
I guess another minor difference is that later calls to Get
and Evaluated
may be slightly faster (no need to acquire the mutex) once slow initialization has been done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I refered to sync.Once
: https://github.com/golang/go/blob/go1.21.4/src/sync/once.go
According to the benchmark below, using atomic is about 10x faster than acquing a lock:
func (l *lazy[T]) Get() T {
if atomic.LoadUint32(&l.done) == 0 {
return l.doSlow()
}
return l.res
}
func (l *lazy[T]) GetWithLock() T {
l.m.Lock()
defer l.m.Unlock()
if l.done == 0 {
l.res = l.getter()
l.done = 1
}
return l.res
}
func BenchmarkGet(b *testing.B) {
lazyFoo := New(func() *foo {
return &foo{}
})
for i := 0; i < b.N; i++ {
lazyFoo.Get()
}
}
func BenchmarkGetWithLock(b *testing.B) {
lazyFoo := New(func() *foo {
return &foo{}
})
for i := 0; i < b.N; i++ {
lazyFoo.GetWithLock()
}
}
goos: linux
goarch: amd64
pkg: antrea.io/antrea/pkg/util/lazy
cpu: Intel(R) Xeon(R) Silver 4114 CPU @ 2.20GHz
BenchmarkGet-4 425120371 2.817 ns/op
BenchmarkGetWithLock-4 44733890 26.91 ns/op
/test-all |
Instead of checking various conditions to determine whether localPodInformer should be initialized, which is error-prone and looks ugly, this patch adds a Generic interface for lazy evaluation and uses it to initialize localPodInformer when it's necessary.