-
Notifications
You must be signed in to change notification settings - Fork 834
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
Memory Mode support: Adding memory mode, and implementing it for Asynchronous Instruments #5709
Memory Mode support: Adding memory mode, and implementing it for Asynchronous Instruments #5709
Conversation
# Conflicts: # sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorage.java
@jack-berg First PR 🎉 |
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 @asafm! This code will definitely reduce the readability of the code, but the performance improvement is so significant that I think its worth it to incur the additional complexity. I left a variety of comments, which are mostly small. I'm very supportive of the overall direction.
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/data/DoublePointData.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricExporter.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricReader.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/ObjectPool.java
Show resolved
Hide resolved
sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/state/PooledHashMapTest.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/ArrayBasedStack.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/ArrayBasedStack.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/ObjectPool.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableLongPointData.java
Show resolved
Hide resolved
…MemoryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…MetricExporter.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…MetricReader.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…l/state/ObjectPool.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
… with other factory methods
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/export/MetricExporter.java
Show resolved
Hide resolved
...metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/data/MutableDoublePointData.java
Outdated
Show resolved
Hide resolved
? ImmutableMeasurement.doubleMeasurement( | ||
start, measurement.epochNanos(), measurement.doubleValue(), processedAttributes) | ||
: ImmutableMeasurement.longMeasurement( | ||
start, measurement.epochNanos(), measurement.longValue(), processedAttributes); |
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.
Measurement withAttributes(Attributes);
Measurement withStartEpochNanos(long);
Wouldn't having 2 methods result in making 2 immutable copies? Perhaps a single
Measurement update(Attributes, long);
method would generate a bit less garbage
...ics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorage.java
Show resolved
Hide resolved
...ics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/AsynchronousMetricStorage.java
Show resolved
Hide resolved
sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/state/MutableMeasurement.java
Outdated
Show resolved
Hide resolved
...ava/io/opentelemetry/sdk/metrics/internal/state/AsynchronousCounterMemoryUsageBenchmark.java
Outdated
Show resolved
Hide resolved
...testing/src/main/java/io/opentelemetry/sdk/testing/exporter/InMemoryMetricReaderBuilder.java
Outdated
Show resolved
Hide resolved
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.
Very close IMO, just a few final cleanups!
sdk/common/src/main/java/io/opentelemetry/sdk/common/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/common/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/common/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/common/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/common/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/common/export/MemoryMode.java
Outdated
Show resolved
Hide resolved
Looks good to me @asafm! Thanks for sticking with it. I'm out for a long weekend but when I get back I can help get the build passing if still stuck. |
…moryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…moryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…moryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…moryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…moryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
…moryMode.java Co-authored-by: jack-berg <34418638+jack-berg@users.noreply.github.com>
@jack-berg the build fails on API compare.
and
and
|
Can you run |
@jack-berg Here's the output. What should I do next?
Commit those files? |
Codecov ReportAttention:
... and 1 file with indirect coverage changes 📢 Thoughts on this report? Let us know!. |
Epic
This is the first PR out of several which implements #5105.
The problem
Issue #5105 describes it quite well, so I'll add a very short motivation.
Currently the SDK uses immutable objects during metric collection, where the bulk of the work is located at the
LeasedMetricProducer
.The side effect of immutable objects is that they generate a lot of garbage collected objects. Given enough instruments and/or attribute sets, this can amount to such a substantial amount, the garbage collector will start to kick in more often, and for longer periods of time.
There are systems which are ultra sensitive to latency, like databases and messaging systems (e.g. Apache Pulsar). Those systems will have a problem using OpenTelemetry as it is today.
The solution
The SDK will have two modes of operation:
What does this PR include?
This PR is focused on adding support for reusability, and implementing it for Asynchronous Instruments.
MemoryMode
enum dictating the two different modes.memoryMode
property toMetricReader
. This is used by the SDK collection process (in this PR only the Asynchronous Instrument related code) to switch between using immutable objects of reusable objects (from a pool).PeriodicMetricReader
we've addedmemoryMode
property toMetricExporter
as well, so both the metric exporter can act upon it, and also the PeriodicMetricReader delegates the value ofmemoryMode
to theMetricExporter
it has.DoublePointData
andLongPointData
. This are used to carry the current value for each (instrument, attributes) pair.Measurement
, by changing it from an abstract class into an interface and creating two classes implementing it:ImmutableMeasurement
andLeasedMeasurement
.SdkObservableMeasurement
has been changed to object reuse if reusable memory mode is selected, for the active metric reader. Since it only createsMeasurement
object only as DTO for callingAsynchronousMetricStorage.record(measurement)
, I've added a singleLeasedMeasurement
object and keep re-using it across calls torecord()
from the callbacks. It was named "leased" since theSdkObserableMeasurement
leases the measurement toAsynchronousMetricStorage
for the duration of therecord()
call - it should not store it hence "leased".Data structures for reusable objects
I've added an
ObjectPool
class which as it sounds, allows you to borrow an object from the pool. If the pool is empty it uses anobjectCreator
function to generate one and return it. When the object is done with, instead of being garbage collected, it is returned back to the pool usingreturnObject(o)
.The pool it self must avoid any O(n) memory allocations upon borrow or return operations, hence using a simple array-based stack I wrote
ArrayBasedStack
. I decided to write one instead of usingArrayDequeue
since I want in next PR improve the extra space it occupies (instead of x2 switching to changing double factor, or trim after x collections)Also I've added
PooledHashMap
, which is a bucket-based HashMap implementation, which uses an object pool when it needs aMapEntry
object. This is needed primarily inAsynchronousMetricStorage
.Storage
AsynchronousMetricStorage
- here the bulk of the work was done in this PR. This is where all the callbackrecord()
calls gets funneled into. The primary changes were:PooledHashMap
instead ofHashMap
to reduceMapEntry
memory allocations which were O(n). The maps are used to store the recordings (attributes -> point) and in case of DELTA temporality, the previous data points.record(measurement)
, the measurement is now converted into a reusable data point, coming from a pool ofreusable data points, in the case of REUSABLE memory mode.
collect()
is now a reusable list (array-based), in the case of REUSABLE memory mode. Array-based removes any internal wrapper objects allocations.collect()
it return a list of reusable points, which needs to return back to the pool. We do so when the next collect() is called. We "save" the list returned, return each reusable data point back to the pool, and reset the list for another usage.The
*Aggregator
classes were modified to allow for reusable data points to avoid allocations.Benchmark tests
Garbage Collection (memory allocation) benchmark
AsynchronousMetricStorageGarbageCollectionBenchmarkTest
runs a JMH benchmark namedAsynchronousMetricStorageGarbageCollectionBenchmark
withgc
profiler, which measures the numberof objects (and size) the garbage collector collected (unused objects).
The
AsynchronousMetricStorageGarbageCollectionBenchmark
benchmark creates 10 asynchronous counters (any asynchronous instrument will do as the code path that was changes is almost the same for all async instrument types), and 1000 attribute sets. Each time the test runs, it callsflush
which effectively calls the callback for each counter. Each such callback records a random number for each of the 1000 attribute sets. The result list ends up inNoOpMetricExporter
which does nothing with it.This is repeated 100 times, collectively called operation in the statistics and each such Operation is repeated 20 times - known as Iteration.
Each such test is repeated, with a brand new JVM, for all combinations of memory mode (REUSBLE_DATA, IMMUTABLE_DATA) and aggregation temporality (CUMULATIVE, DELTA). This is done since each combination has a different code path.
The statistics of this run can be seen in the output of the test and also pasted here below:
If we focus only on the normalized rate:
You can see that for DELTA aggregation temporality, REUSABLE_DATA scores a rate of 257,078 [bytes/operation] compared with 271,877,200 [bytes/operation], which means REUSABLE_DATA memory mode reduced the size of
garbage collected objects by 99.9% compared with IMMUTABLE_DATA memory mode. Similar results (99.88%) exists for CUMULATIVE.
The test verifies that. Aside from the convenience of running this inside your code editor with one click, it protects from any future regression that can easily arise from a simple refactor or change.
Memory usage benchmark
IMMUTABLE_DATA "pays" with lots of one-off objects which almost immediately gets garbage collected. Memory is allocated to certain peak and then released. On the other hand, REUSABLE_DATA uses reusable objects, located in object pools, which stays in memory even after the collection finished. So it "pays" with constant memory usage. In a perfect implementation the peak memory consumption would almost be equal in both cases.
The object pools are not yet tuned, so they aggressively allocates more than what they need. In subsequent PRs I would like to minimize that effect, so the max heap for both is not far off each other.
You can run
AsynchronousCounterMemoryUsageBenchmark
main()
method to view the memory usage for each combination of aggregation temporality, memory mode, number of asynchronous instruments and number of attribute sets.The results are:
Cumulative
Delta
So you can see for example that for 50 counters and 100k attribute sets each, the maximum used memory is 958mb for reusable data and 581mb for immutable data, meaning 64% more memory needed. For 1 instrument, it's 20%. The reason it's bigger percentage as it has more attribute sets is because currently the object pool grows by 2 each time it runs out of capacity. This is something to be be tuned in next PRs.
opentelemetry-sdk-metrics.txt
opentelemetry-sdk-testing.txt
Future PRs