Use sync.Map in the endpointregistry #2795
Conversation
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
from
91e0638 to
95ecd0f
Compare
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
from
95ecd0f to
0eb1384
Compare
|
Please add a benchmark showing performance and allocations of reading metrics (especially detected time). |
|
@AlexanderYastrebov I posted the benchmark inside PR description. |
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
2 times, most recently
from
b576936 to
d229720
Compare
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
from
d229720 to
92d1657
Compare
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
4 times, most recently
from
d37a6a0 to
a599912
Compare
routing/routing.go
Outdated
|
|
||
| // There is ongoing effort to move data from Metrics and Detected to this field to simplify addition of new metrics | ||
| // and to make them host-wide instead of route-wide | ||
| MetricsFromRegistry atomic.Value // Metrics |
There was a problem hiding this comment.
This field is:
- stored by endpointregistry postprocessor
- loaded by fadein postprocessor
- will be loaded by loadbalancer
I am not sure whether it is safe to store and load concurently (even when stored value is equal to the current value), so I have decided to make this field atomic.
There was a problem hiding this comment.
Lets add this field in a separate PR. It also does not have to be atomic.Value because it is only read by proxy
There was a problem hiding this comment.
As far as I will be able to resolve comment without this field, I am able to add it in the separate PR.
#2795 (comment)
filters/fadein/fadein.go
Outdated
| @@ -235,6 +235,7 @@ func (p *postProcessor) Do(r []*routing.Route) []*routing.Route { | |||
| } | |||
|
|
|||
| ep.Detected = detected | |||
| ep.MetricsFromRegistry.Load().(routing.Metrics).SetDetected(detected) | |||
There was a problem hiding this comment.
This should set value to the registry similar the code above
metrics := r.GetMetrics(ep.Host)
if endpointsCreated[key].After(metrics.DetectedTime()) {
metrics.SetDetected(endpointsCreated[key])
}or something like that
There was a problem hiding this comment.
I have to introduce additional argument to the fadein postprocessor constructor then, because there is no other way to pass endpointregistry here:
https://github.com/zalando/skipper/pull/2795/files#diff-9eb07484b61cb4592f195fc77b141796af8db69d01115364678c0de48abd6c70R202
There was a problem hiding this comment.
Pre- and PostProcessors are normally fine to change.
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
from
a599912 to
1ab3dfe
Compare
Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
RomanZavodskikh
force-pushed
the
endpointregistrySyncMap
branch
from
1ab3dfe to
9c13c27
Compare
| e.lastSeen.Store(ts) | ||
| } | ||
|
|
||
| func newEntry() (result *entry) { |
There was a problem hiding this comment.
why named return value?
This looks much better:
func newEntry() *entry {
result := &entry{}
result.SetDetected(time.Time{})
result.SetLastSeen(time.Time{})
return result
}
| return &EndpointRegistry{ | ||
| data: map[string]*entry{}, | ||
| lastSeen: map[string]time.Time{}, | ||
| result := &EndpointRegistry{ |
There was a problem hiding this comment.
why store result if you can just return it?
|
@RomanZavodskikh more nits than very important findings. 👍 |
| } | ||
|
|
||
| func BenchmarkGetDetectedTime(b *testing.B) { | ||
| goroutinesNums := []int{1, 2, 3, 4, 5, 6, 7, 8, 12, 16, 24, 32, 48, 64, 128, 256, 512, 768, 1024, 1536, 2048, 4096} |
There was a problem hiding this comment.
Why stop at 4096 and not 8192?
I see one cluster has regular close to 10k goroutines and one outlier cluster that had in the last 8h 31k.
So maybe 32768 makes even sense to test.
There was a problem hiding this comment.
The benchmarks are fine up to 32k goroutines, I will update results and this code in the next PR.
|
👍 |
|
The loadtests of this PR showed it is good to go, so will merge now. |
|
👍 |
| if options.EndpointRegistry == nil { | ||
| options.EndpointRegistry = routing.NewEndpointRegistry(routing.RegistryOptions{}) | ||
| } |
There was a problem hiding this comment.
This makes no sense as this registry instance will not be visible outside of this postprocessor.
The code should call GetMetrics only when registry is not nil instead.
#2795 Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
#2795 Signed-off-by: Roman Zavodskikh <roman.zavodskikh@zalando.de> Co-authored-by: Roman Zavodskikh <roman.zavodskikh@zalando.de>
EndpointRegistry.GetMetrics always allocates new entry even when data for the key already exists. sync.Map does not provide lazy initialization for new value, see golang/go#44159 This change uses suggested best-effort to avoid allocating new value. Follow up on #2795 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
EndpointRegistry.GetMetrics always allocates new entry even when data for the key already exists. sync.Map does not provide lazy initialization for the new value, see golang/go#44159 This change uses suggested best-effort to avoid allocating a new value on each load. Follow up on #2795 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
EndpointRegistry.GetMetrics always allocates new entry even when data for the key already exists. sync.Map does not provide lazy initialization for the new value, see golang/go#44159 This change uses suggested best-effort to avoid allocating a new value on each load. Follow up on #2795 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
…2847) EndpointRegistry.GetMetrics always allocates new entry even when data for the key already exists. sync.Map does not provide lazy initialization for the new value, see golang/go#44159 This change uses suggested best-effort to avoid allocating a new value on each load. Follow up on #2795 Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
This PR is intended to make endpointregistry component lock-free
The benchmark results show that it could be slow to call
GetMetricstoo much, but if the user will find a way to reuse its return value, the performance will be much better both than current master and new version: