envoyproxy / envoyproxy/envoy

stats: excessive ref-count inc/dec causing performance bottleneck

Open
#43,836 9 comments 0 reactions 1 assignee Claimed by @jmarantz View on GitHub
area/perf area/stats no stalebot
Dominant language
C++
Stars
28.9k
Forks
5.6k
Avg merge
1d 22h
Merged PRs (30d)
430

Description

We have anecodotal flame-graphs in one application of Envoy where the destruction of MetricSnapshotImpl on the main thread hot-spots in the ref-count decrement code. Context is that there are a very large numbers of stats, and a decent number of threads.

The hot-spot appears to be due to https://github.com/envoyproxy/envoy/pull/18144 which "temporarily" (in Sept 2021 :)) added these references to the MetricSnapshotImpl when collecting the snapshot as insurance that the stats could not be deleted while iterating over them and passing them to the sink. It promised to remove this when switching to iterators to pass the stats to sinks, but this has not occurred.

And I think making that conversion will be a little difficult (not impossible) because:
* this is in the public Sink interface which has many derived classes in the Envoy codebase and in private repos; changing that interface to be iterator based rather than assuming vectors are available would be fairly toily. Maybe AI could help.
* The Store and Scope iterators have to run with a lock held, and that might cause deadlocks if the sink handler needs to do a stats operation that requires a lock. This seems intuitively unlikely; it would probaby be fine. But hard to prove. The Store iterator uses a lock in the Allocator, and the Scope iterator uses a lock in the Store, interestingly. Either way there's a deadlock risk depending on what happens in the sink. This would need to be system-tested for every sink.

Several other possible remedies exist, and we may be able to use more than one of them:
* make the atomic operations on refcount inc/dec more effiicient; the increment can likely have relaxed ordering but the decrement probably has to have a strong barrier. This needs careful thinking, and I think would be somewhat incremental in benefit. This could be done in combination with the other two options below, and we could also do that for the stat values themselves, which probably can tolerate a more relaxed model.
* Simply revert #18144 which proclaims itself as unnecessary, because the metrics are collected in the main thread, and the main thread is also where the stats would eventually be reaped if someone drops a scope during the sink process @pradeepcrao WDYT?
* Instead of holding onto a reference in each stat, just hold onto references to the scopes. The Scope defines the lifetime of the stats held within, and this probably gives us a 100x benefit relative to the current state. I think that at the time #18144 was written, scopes were not held as shared_ptr, so refs to each individual stat were kept. Holding scopes as shared_ptr was added in https://github.com/envoyproxy/envoy/pull/19791 in 2022.

@ggreenway @kyessenov may also have opinions.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.