Avoid unnecessary cache building for cachingCost #12465
Conversation
There was a problem hiding this comment.
Thanks for trying to solve this, @AmatyaAvadhanula !
I would suggest breaking up this PR into two parts.
- First for interned intervals. This would require a perf evaluation of how interning the interval affects both computation time and memory footprint.
- Second for removing the cache building step. This would require an analysis/proof of how the cost computed by the new method is the same as that computed by the existing method using the cache chain. There should be tests around this proof too. It would also be nice to have some perf evaluation in this PR as there would be a marked decrease in the number of ephemeral objects created and also compute times.
| @@ -80,6 +79,12 @@ public final class SegmentId implements Comparable<SegmentId> | |||
| */ | |||
| private static final Interner<String> STRING_INTERNER = Interners.newWeakInterner(); | |||
|
|
|||
| /** | |||
| * Store Intervals since creating them each time before returning is an expensive operation | |||
| this.intervalStartMillis = interval.getStartMillis(); | ||
| this.intervalEndMillis = interval.getEndMillis(); | ||
| this.intervalChronology = interval.getChronology(); | ||
| this.interval = INTERVAL_INTERNER.intern(interval); |
There was a problem hiding this comment.
Can interval ever be null here?
If not, we can add Objects.requireNonNull similar to the datasource validation in the previous line.
| @@ -70,10 +70,19 @@ protected double computeCost(DataSegment proposalSegment, ServerHolder server, b | |||
| return cost * (server.getMaxSize() / server.getAvailableSize()); | |||
| } | |||
|
|
|||
| private ClusterCostCache costCacheForLoadingSegments(ServerHolder server) | |||
| private double costCacheForLoadingSegments(ServerHolder server, DataSegment proposalSegment) | |||
There was a problem hiding this comment.
Nit: Rename to computeCostForLoadingSegmentOnServer
|
Doing just the interval interning would make this mergeable, definitely do a separate PR for the caching as the correctness of that is less clear. We should probably include the flamegraphs that led us to make this code change in this PR. In terms of memory consumption, the fields being stored on SegmentId are the exact same as what an Interval stores, by interning and reusing the same reference, given that the same interval tends to show up a lot, we should actually save on memory consumption versus increase it while also improving performance. |
|
Closing since #14484 deprecates cachingCost |
Description
CachingCostBalancerStrategy can be inefficient when there are a large number of segments in the load / drop queue.
It builds a cache which takes O(N ^ 2) and computes it N times in the process of loading N segments.
This can be avoided by simply computing and adding the pairwise costs in O(N) computed N times.
Key changed/added classes in this PR
CachingCostBalancerStrategyThis PR has: