Reduce []ConfigKey slice allocate numbers during xds caching
#39731
Conversation
…ead which will cleanup in background
…try this could largely release garbage collector pressurere
|
Only need to look at the latest commit 0a6f271 |
|
/test benchmark_istio |
|
|
||
| // In order to save cpu, if the evicted keys is very small, we can just ignore | ||
| // TODO: make it configurable | ||
| if len(l.evictedKeys) < 100 { |
There was a problem hiding this comment.
is the optimization worth the inaccuracy? I think things like delta will depend on whether or not entries in the cache are still valid. We could just check the cache AND evictedKeys though -- @howardjohn
There was a problem hiding this comment.
Not sure i understand
There was a problem hiding this comment.
This can be seen as a debouncer, hope you can better understand
| func (b EndpointBuilder) DependentConfigs(allocator *model.Allocator) []model.ConfigKey { | ||
| var configs []model.ConfigKey | ||
| if allocator != nil { | ||
| configs = allocator.Allocate(2) |
There was a problem hiding this comment.
At most 2 is needed for EndpointBuilder
| if features.EnableXDSCacheMetrics { | ||
| xdsCacheEvictions.Increment() | ||
| } | ||
|
|
||
| key := k.(string) | ||
| value := v.(cacheValue) | ||
| if l.trackEvicted { |
There was a problem hiding this comment.
in what scenario do we not want to track evicted? do we just not evict if this is not set?
|
@hzxuzhonghu should #39688 be closed in favor of this? |
|
Intend to make #39688 first as it is almost there. But the commit is largely dependent on previous pr. |
|
@hzxuzhonghu: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
As mentioned before, I don't feel comfortable merging these types of performance changes, which may or may not bring improvement, without numbers actually showing differences.
Currently our benchmarks only show a regression from this change, so if the are not giving accurate coverage then please update them.
|
i have to say can we be fair? addind dependentconfigs to cache value does not improve any perf, but on opposite mem costing.. i just reverted the bad stuff in #39688 |
Please provide a description of this PR:
This is making use of sync.Pool to relieve pressure on the garbage collector.