xiangfu0 commented on code in PR #19530:
URL: https://github.com/apache/pinot/pull/19530#discussion_r4009947696
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java:
##########
@@ -45,11 +47,13 @@ public class BalancedInstanceSelector extends
BaseInstanceSelector {
@Override
public InstanceMapping select(List<String> segments, int requestId,
SegmentStates segmentStates, Map<String, String> queryOptions) {
- Map<String, String> segmentToSelectedInstanceMap = new
HashMap<>(HashUtil.getHashMapCapacity(segments.size()));
+ // Allocate the flat map only when a required segment is selected. It
avoids one map node per segment without
+ // reserving large arrays for queries whose segments are all optional or
unavailable.
+ Map<String, String> segmentToSelectedInstanceMap = null;
Review Comment:
The relevant case is the query's selected subset: it can contain only
optional or unavailable segments while other table segments are online. HashMap
defers its table allocation until insertion, whereas Object2ObjectOpenHashMap
allocates both arrays in its constructor. For an expected 10,000 entries, those
unused arrays would be about 131 KB on the tested JVM.
I kept the guard to preserve the empty-required behavior and clarified this
in both selectors. The isolated map benchmark uses the same guard for both map
types, so its populated-map comparison does not attribute any gain to laziness.
The all-optional and all-unavailable controls are included in [the benchmark
evidence](https://gist.github.com/xiangfu0/7acc19615156089a08cb49b7e386b524).
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -416,6 +416,8 @@ private void processInstanceConfigChangeInternal() {
String instanceId = instanceConfigZNRecord.getId();
try {
if (isEnabledServer(instanceConfigZNRecord)) {
+ // Match the interned instance IDs decoded from IS/EV map keys for
per-segment routing lookups.
+ instanceId = instanceId.intern();
Review Comment:
Agreed that Guava [generally recommends a weak
interner](https://guava.dev/releases/33.4.8-jre/api/docs/com/google/common/collect/Interner.html).
Here, Helix/Jackson already interns assignment-map IDs as JSON field names in
the JVM pool, while config IDs are JSON values. This call joins that existing
pool so the enabled-server map keys share identity with the assignment IDs used
in per-segment lookups. Jackson's [field-name
defaults](https://github.com/FasterXML/jackson-core/blob/jackson-core-2.22.2/src/main/java/com/fasterxml/jackson/core/JsonFactory.java#L70-L94)
and
[InternCache](https://github.com/FasterXML/jackson-core/blob/jackson-core-2.22.2/src/main/java/com/fasterxml/jackson/core/util/InternCache.java#L43-L71)
establish that behavior.
Replacing only this call with Guava would preserve correctness but lose that
identity fast path: a [Guava interner uses its own
map](https://github.com/google/guava/blob/v33.4.8/guava/src/com/google/common/collect/Interners.java#L108-L143).
A shared domain interner would need adoption on both the config and assignment
paths, with its refresh cost measured separately. I kept that broader change
outside this PR, clarified the comment, and strengthened the regression test to
route with IDs from actual assignment-map deserialization and verify they are
the enabled-map key objects across config refreshes.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/manager/BaseBrokerRoutingManager.java:
##########
@@ -1193,30 +1195,34 @@ public RoutingTable getRoutingTable(BrokerRequest
brokerRequest, String tableNam
private Map<ServerInstance, SegmentsToQuery>
getServerInstanceToSegmentsMap(String tableNameWithType,
InstanceSelector.SelectionResult selectionResult) {
Map<ServerInstance, SegmentsToQuery> merged = new HashMap<>();
- for (Map.Entry<String, String> entry :
selectionResult.getSegmentToInstanceMap().entrySet()) {
- ServerInstance serverInstance =
_enabledServerInstanceMap.get(entry.getValue());
+ // Flat selection maps can traverse their arrays directly without
allocating an entry object per segment.
+ selectionResult.getSegmentToInstanceMap().forEach((segment, instanceId) ->
{
+ ServerInstance serverInstance =
_enabledServerInstanceMap.get(instanceId);
if (serverInstance != null) {
- SegmentsToQuery segmentsToQuery =
- merged.computeIfAbsent(serverInstance, k -> new
SegmentsToQuery(new ArrayList<>(), new ArrayList<>()));
- segmentsToQuery.getSegments().add(entry.getKey());
+ SegmentsToQuery segmentsToQuery = merged.get(serverInstance);
+ if (segmentsToQuery == null) {
+ segmentsToQuery = new SegmentsToQuery(new ArrayList<>(), new
ArrayList<>());
+ merged.put(serverInstance, segmentsToQuery);
+ }
+ segmentsToQuery.getSegments().add(segment);
Review Comment:
You're right: get/put adds a lookup for each server's first segment. I
isolated it from the map/counter changes and tested both workload sizes. At
10,000 segments it helped eight-worker routing throughput, but at 100 segments
it consistently hurt: balanced -12.90% / -8.23% and replica group -3.44% /
-7.27% across forward/reverse orders. Both forms already create each server's
lists once.
Restored `computeIfAbsent` in 8b38601012 to avoid that small-request
regression. The final implementation was separately compared against the
interning-only baseline; all 24 BrokerRoutingManagerTest tests pass. [Full
isolated results and
reproduction](https://gist.github.com/xiangfu0/7acc19615156089a08cb49b7e386b524).
##########
pinot-broker/src/main/java/org/apache/pinot/broker/routing/instanceselector/BalancedInstanceSelector.java:
##########
@@ -72,20 +76,24 @@ public InstanceMapping select(List<String> segments, int
requestId,
} else {
selectedCandidate = candidates.get(requestId++ % candidates.size());
}
- poolToSegmentCount.merge(selectedCandidate.getPool(), 1, Integer::sum);
+ poolToSegmentCount.addTo(selectedCandidate.getPool(), 1);
// This can only be offline when it is a new segment. And such segment
is marked as optional segment so that
// broker or server can skip it upon any issue to process it.
if (selectedCandidate.isOnline()) {
+ if (segmentToSelectedInstanceMap == null) {
+ segmentToSelectedInstanceMap = new
Object2ObjectOpenHashMap<>(segments.size());
Review Comment:
I ran a map-only comparison before deciding: HashMap versus
Object2ObjectOpenHashMap with identical primitive counters, lazy guard,
grouping, direct traversal, canonical IDs and dependencies. Both selectors,
100/10,000 segments, one/eight workers, forward/reverse order, four fresh JVM
forks per case overall, GC profiler.
At 10,000 ONLINE segments the flat map saves about **254,440 B per routing
call** (40.7–41.1% of total allocation in this isolated comparison).
Eight-worker throughput is close to neutral/mixed: balanced +4.27% / -0.42%,
replica group -0.46% / -1.09%. At 100 segments, throughput improves in both
orders: balanced +6.64% / +15.27%, replica group +6.20% / +18.30%.
Single-worker large balanced selection is slightly slower (+3.12% / +0.50%
elapsed time); this is not a universal timing win.
The sparse tradeoff is real: when 99% of 10,000 inputs are optional or
unavailable, it allocates about **62.4 KB more/call** because sizing uses all
inputs. A short optional-99 timing spike did not recur in a separate
full-protocol confirmation; the original spike and extra allocation are
retained in the report. The all-optional/all-unavailable cases avoid
required-map allocation through the guard.
I kept the flat map for the populated-query allocation savings and
small-query results, and documented the sparse cost in the PR. These are
allocated bytes/call, not retained heap. The separately measured get/put
regression has been removed. [Raw data, all controls, exact source differences
and
reproduction](https://gist.github.com/xiangfu0/7acc19615156089a08cb49b7e386b524).
--
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]
