Get rid of unnecessary allocations in certain Duration/DataSize methods #11
Conversation
When succinctBytes is hot it can allocate plenty of DataSize instances. In Presto, we observed that DataSize instance allocations were responsible for ~4% of all allocations on the coordinator.
To create a duration with the most succint time unit typical usage is "new Duration(value, unit).convertToMostSuccinctTimeUnit()" or the succinctDuration method. However, both methods require an allocation of a Duration instance and they can add up when the calling method is hot. In Presto we observed that these account for ~3.5% of all allocations on the coordinator. This change adds a private static toSuccinctDuration method, which is used from succinctDuration, to get rid of the unnecessary allocation.
| @@ -45,7 +45,7 @@ public static DataSize succinctBytes(long bytes) | |||
|
|
|||
| public static DataSize succinctDataSize(double size, Unit unit) | |||
| { | |||
| return new DataSize(size, unit).convertToMostSuccinctDataSize(); | |||
| return convertToMostSuccinctDataSize(size, unit); | |||
There was a problem hiding this comment.
Have you measured the effect of this change? The VM should be able to elide this allocation, so I'm wondering if you're seeing some other effect, instead. Also, how did you measure the allocations?
There was a problem hiding this comment.
I used async profiler to do the allocation profiling. For this particular method I don't see anything in profiler output, so I think you are right that these allocations are elided, but the confusing part is for the similar Duration::succinctDuration method the local Duration allocations are not elided as they show up in profiler output (sample output is below). So, my conclusion was these allocations are not always elided that's why I created this PR.
--- 733370120 bytes (0.15%), 66 samples
[ 0] io.airlift.units.Duration
[ 1] io.airlift.units.Duration.succinctDuration
[ 2] io.airlift.units.Duration.succinctNanos
[ 3] io.airlift.units.Duration.nanosSince
|
@martint any other thoughts on this one? |
This PR gets rid of unnecessary memory allocation in
Duration::succinctDurationandDataSize::succinctBytesmethods.