Reuse MetricData #5178
Reuse MetricData #5178
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5178 +/- ##
============================================
- Coverage 90.97% 90.93% -0.05%
- Complexity 4907 4941 +34
============================================
Files 552 556 +4
Lines 14489 14593 +104
Branches 1372 1374 +2
============================================
+ Hits 13182 13270 +88
- Misses 907 919 +12
- Partials 400 404 +4
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
...c/main/java/io/opentelemetry/sdk/metrics/internal/state/DefaultSynchronousMetricStorage.java
Outdated
Show resolved
Hide resolved
…y-java into reuse-metric-data
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
| if (reset) { | ||
| buckets.clear(); | ||
| } | ||
| return copy; | ||
| return mutableBuckets; |
There was a problem hiding this comment.
This was one idea I entertained for awhile performance gain. How are you avoiding multi-threads touching this data?
Is it because you're only returning this to ONE metric-reader at a time and the "hot path" of writes is still writing to the underlying data allocated in this handle?
If so, VERY clever. We should document this in the handle class how it works and why it's safe.
There was a problem hiding this comment.
Is it because you're only returning this to ONE metric-reader at a time and the "hot path" of writes is still writing to the underlying data allocated in this handle?
Yes exactly. While we support multiple readers, we don't support concurrent reads. As long as readers don't hold on to references to MetricData and try to read after they're done reading, they shouldn't get any weird behavior. Right now this won't work with multiple readers since once PeriodicMetricReader calls MetricProducer#collectAllMetrics(), another reader will be able to start reading and MetricData will be mutated out from under the PeriodicMetricReader. Ouch. But this is solvable by providing readers a way to communicate to MetricProducer that they're done consuming the data. For example, by adjusting collectAllMetrics to accept a CompletableResultCode which the reader completes when finished consuming the data, i.e. MetricProducer#collectAllMetrics(CompleteableResultCode).
As you noticed, this also relies on different objects for writes vs. reads (writes use AggregationHandle, reads use some some mutuable MetricData).
There was a problem hiding this comment.
Scratch that part about readers needing to communicate when they're finished consuming the data. Each reader has its own copies of metric storage, and the mutable MetricData, so its much simpler: It should be safe as long as a MetricReader doesn't hold on to the MetricData references and try to consume them during a subsequent collect.
…y-java into reuse-metric-data
|
Closing since #5709 has been merged. |
This is POC that pushes the effort to reduce memory allocation to its limit by reusing all data carrier classes on repeated collections (i.e.
MetricData,PointData, supporting arrays etc). I've prototyped this on the exponential histogram aggregation, which is the most complicated.We can arguably do this safely because readers aren't allowed to perform concurrent reads, so if they synchronously consume all the data during collection and export, there's no risk of the data being updated out from under them. Could also make this explicit by adding a method to
MetricReader/MetricExporterthat indicated the desired memory behavior, where the default is to make immutable data carriers as we do today, while allowing for opting in to this improved alternative.The memory allocation pretty close to as low as possible with this change. The only remaining allocations I see when profiling are allocations for iterators like this, which would be hard to get rid of.
Performance results before:
And after:
The aggregate reduction of memory between this and the other changes is quite impressive. The default exp histogram aggregation with cumulative temporality has reduced from an original bytes / op of
46_466_259to110_198with this PR. A 99.8% reduction, and 420x improvement!I've run this locally with an app that produces 1_000_000 unique series, and its pretty impressive how little memory is allocated on collect. Something like 25mb per collection, or 25 bytes per series. Immutability is great, but it's hard to ignore these performance gains!