Jackie-Jiang commented on code in PR #19915:
URL: https://github.com/apache/pinot/pull/19915#discussion_r4383150890
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/ReplicaGroupInstanceSelector.java:
##########
@@ -73,6 +75,45 @@ public class ReplicaGroupInstanceSelector extends
BaseInstanceSelector {
private static final Logger LOGGER =
LoggerFactory.getLogger(ReplicaGroupInstanceSelector.class);
+ // Immutable key for instance groups that reduces hash collisions.
+ static final class InstanceSetKey implements Comparable<InstanceSetKey> {
+ private final String[] _instances;
+ private final int _hashCode;
+
+ InstanceSetKey(Set<String> instances) {
+ _instances = instances.toArray(new String[0]);
+ Arrays.sort(_instances);
Review Comment:
Can we avoid the extra copy per entry/lookup here? Each `InstanceSetKey`
construction allocates a wrapper, copies the instance names into a new array,
and sorts it. We do this twice per processed old `InstanceStateMaps`
(collection and lookup), and once per new segment. When candidate sharing is
exhausted or assignments are distinct, that becomes two constructions per old
segment. Even with small replica counts, this adds allocation and GC work
across large routing rebuilds.
The normal assignment writers create natural-order `TreeMap`s, and the Helix
serialization/deserialization path preserves their iteration order even when
the result is a `LinkedHashMap`. We should be able to use the existing
`idealStateInstanceStateMap.keySet()` views directly with an
`Object2ObjectOpenCustomHashMap<Set<String>, Set<String>>` and a custom
`Hash.Strategy`:
- Compute the hash in iteration order, retaining the same mixed fold used
here:
```java
int hash = 1;
for (String instance : instances) {
hash = 31 * hash + HashCommon.mix(instance.hashCode());
}
```
- Compare sizes and then compare the two iterators element by element. Only
instance names participate; state values must not affect grouping.
- Handle identity/nulls in the strategy, since fastutil also calls
`equals(key, null)` internally.
This removes the wrapper, array copy, and sort while retaining the same hash
for naturally ordered inputs. The map is local to the rebuild, so the backing
assignment maps only need to remain unchanged for that lifetime. Please
document the natural-order iteration precondition and cover the actual
assignment/serialization round trip in a test; account for the existing
shuffled-map tests when defining that contract.
Could you also share some before/after performance numbers? The
hash-distribution test demonstrates fewer collisions, but does not measure the
rebuild cost or the added allocation. Reusing the workload from #19852 (380k
segments, 200 servers, 3 replicas) would make the results comparable:
- End-to-end `onAssignmentChange` time and allocated bytes per rebuild, with
GC time/count if available.
- A few distinct-instance-set counts, such as 64, 4,096, 50,000, and
380,000, to cover both heavily shared and mostly distinct assignments.
- Healthy assignments and a case with unavailable instances, using the same
JVM/configuration and reporting the candidate-sharing limit.
- Baseline versus this PR; include the custom-strategy version if you try
it. Please use warmup and repeated measurements so we can see the variation.
This would show both the collision-related improvement and whether the extra
copying adds meaningful overhead in the common case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
