perf(executor): hot-path micro-optimizations and collision-safe deduplication#2158
perf(executor): hot-path micro-optimizations and collision-safe deduplication#2158Copilot wants to merge 2 commits into
Conversation
…lication Phase 1 - Hot-path hygiene: - Replace Object.keys().length check with allocation-free hasOwnProperties() - Replace Array.from().some() with for-of loop and early return - Replace filter().map() errorPath chain with single-pass loop - Merge filter().some() + iteration loop in executeFetchPlanNode into one pass Phase 2 - Allocation churn: - Refactor traverseFlattenPath to index-based iteration (eliminates [segment,...rest] array per step) - Replace slice()+spread in error path relocation with pre-sized array appends - Fix entityPathsByRepresentationIndex create-once pattern Phase 3 - Collision-safe deduplication: - Add canonicalEncodeEntity() with type tags, sorted keys, undefined exclusion, safe fallback - Add findInBucket() for collision bucket lookup - Use hash+bucket Map<number,Array<[string,number]>> for both flatten and batch dedup Co-authored-by: kamilkisiela <8167190+kamilkisiela@users.noreply.github.com> Agent-Logs-Url: https://github.com/graphql-hive/gateway/sessions/3adeccbc-4483-4cf9-ad4f-9c59f7ab8e6e
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving the runtime performance and correctness of federated execution in packages/router-runtime/src/executor.ts by reducing hot-path allocations, lowering GC pressure during deep flatten traversals, and making entity deduplication robust against 32-bit hash collisions.
Changes:
- Replaced allocation-heavy patterns (e.g.,
Object.keys().length,Array.from().some(), filter/map chains) with allocation-free or single-pass loops on hot paths. - Refactored
traverseFlattenPathto index-based traversal to avoid per-recursion array allocations; optimized error-path relocation to avoidslice()/spread churn. - Introduced collision-safe entity deduplication via canonical encoding + per-hash collision buckets.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // New hash — definitely new entity | ||
| dedupIndex = dedupedRepresentations.length; | ||
| representationKeyToIndex.set(dedupeKey, dedupIndex); | ||
| const canonical = canonicalEncodeEntity(representation); | ||
| hashToEntry.set(hashKey, [[canonical, dedupIndex]]); | ||
| dedupedRepresentations.push(representation); |
There was a problem hiding this comment.
On the common path (first time a hashKey is seen), this still computes canonicalEncodeEntity(representation) and stores it. Since collisions should be rare, consider storing only dedupIndex for new hashes and computing/storing canonicals lazily only when a second representation hits the same hashKey (then compute canonical for both entries).
| // New hash — definitely new entity | ||
| dedupIndex = ensuredVariableBatchState.representations.length; | ||
| ensuredVariableBatchState.identityToEntityIndex.set( | ||
| identity, | ||
| dedupIndex, | ||
| ); | ||
| const canonical = canonicalEncodeEntity(representation); | ||
| ensuredVariableBatchState.identityToEntityIndex.set(hashKey, [ | ||
| [canonical, dedupIndex], |
There was a problem hiding this comment.
Even when a hashKey is new, this computes/stores canonicalEncodeEntity(representation). To keep the no-collision case allocation-light, consider deferring canonical encoding until a hash bucket already exists, and only then materializing the canonicals needed for collision checks.
| const hashKey = stableStringify(representation); | ||
| let bucket = hashToEntry.get(hashKey); |
There was a problem hiding this comment.
stableStringify() now returns a 32-bit hash (number) rather than a stringified representation, but the name suggests it returns a string. Renaming it (and corresponding call sites like hashKey = ...) to something like hashValue32/stableHash32 would make the dedupe logic easier to understand and reduce the risk of accidental misuse later.
| function canonicalEncodeEntity(value: unknown): string { | ||
| if (value === null) { | ||
| return 'N'; | ||
| } | ||
| const t = typeof value; |
There was a problem hiding this comment.
The new collision-safe dedupe behavior (canonicalEncodeEntity + bucketed Map<number, Array<[string, number]>>) is correctness-sensitive but currently appears untested in this package. Please add tests that exercise (1) distinct representations that share a hash bucket are not merged, and (2) type-tagging/undefined-key exclusion semantics (e.g. "1" vs 1, and {a: undefined, b: "2"} vs {b: "2", c: undefined}).
Applies OTel-style hot-path hygiene to
executor.ts: eliminate hidden allocations on critical federation execution paths, reduce GC pressure in deep-traversal scenarios, and make entity deduplication collision-safe.Phase 1 — Hot-path hygiene
Object.keys().length→hasOwnProperties(): avoids allocating a throwaway array just to check emptiness inhandleResp().Array.from().some()→for...ofwith early break: stops materializing the full iterator into an array inprepareBatchFetchContext.filter().map()→ single-pass loop:errorPathconstruction inprepareFlattenContextnow accumulates in one pass.filter().some()+ loop merger: the type-condition pre-filter and payload-building loop inexecuteFetchPlanNodeare folded into one pass with an earlybreak.Phase 2 — Allocation churn
traverseFlattenPathindex-based refactor: eliminatedconst [segment, ...rest] = remainingPathon every recursive step. The function now takes(pathSegments, pathIndex)and advances withpathIndex + 1; no per-step array allocation on deep traversals of large federated responses.errorPath.slice(tailStart)+[...mappedPath, ...tail]spreads in bothnormalizeBatchFetchErrorsandnormalizeFetchErrorswith a singlenew Array(mappedPath.length + tailLen)filled via index loops.entityPathsByRepresentationIndex: replacedget() ?? []+ unconditionalset()on every entity with a true allocate-once pattern.Phase 3 — Collision-safe deduplication
The 32-bit hash used as a dedup key had no collision detection. Two different entity representations with the same hash would be incorrectly merged.
Introduced
canonicalEncodeEntity()— a type-tagged canonical encoder — andfindInBucket()for collision resolution. Both flatten and batch dedup maps now useMap<number, Array<[string, number]>>(hash → collision bucket):canonicalEncodeEntityuses type tags (s,n,b1/b0,A,O) so"1"and1, or"true"andtrue, are never equal. Object keys are sorted andundefinedvalues are excluded before computing the key count prefix, preventing false collisions like{a: undefined, b: "2"}vs{b: "2", c: undefined}. AJSON.stringifyfallback covers unknown/future types.Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
rover.apollo.dev/opt/hostedtoolcache/node/24.14.0/x64/bin/node /opt/hostedtoolcache/node/24.14.0/x64/bin/node ./install.js(dns block)If you need me to access, download, or install something from one of these locations, you can either:
Original prompt
Implement a comprehensive performance improvement plan for
executor.tsfollowing the OTel-inspired methodology, targeting allocations and hot-path micro-optimizations in federated GraphQL execution. The work is split into these phases:Phase 1 — Low-risk hot-path hygiene
Object.keys(...).length) with allocation-free patterns (tracked boolean orhasOwnPropertyscan).Array.from(...).some, filter/map chains with manual single-pass loops.Mapset/get patterns to allocate only as needed (create-once per key).Phase 2 — Recurring allocation churn
traverseFlattenPath(and similar) to use index-based iteration, eliminating[segment, ...rest]destructuring arrays.slice()or spread snapshotting in critical traversals (collectFlattenEntities, batch error relocation, etc.).filter(...).some(...)) on performance-relevant paths.Phase 3 — Dedupe key optimization
Phase 4 — Plan-level/memoized improvements
normalizeFlattenNodePath) if profiling validates plan reuse and performance gain.General:
The PR should deliver all of the above, targeting the
kamil-random-perfbranch, and should be suitable to pair with or supersede improvements in #2119.The following is the prior conversation context from the user's chat exploration (may be truncated):
User: Yep — I’d turn this into a focused perf plan, but I’d widen it a bit beyond the first obvious fixes.
The core lesson from the two OTel PRs is pretty consistent: hot-path code should avoid convenience helpers that allocate or hide work. #6515 sped up hashAttributes() by avoiding Array.map() and most JSON.stringify() use, with benchmarked gains from about 2.1x to 10x depending on attribute shape. The review also called out an important nuance: keep a safe fallback for future/unexpected value types instead of hardcoding only today’s cases. #6516 pushed the same philosophy into tracing by removing Object.entries(...), avoiding Object.keys(...).length in loops, and using simpler counters/loops; its posted benchmarks improved addEvent/addLink with 128 attributes from roughly 9.5k ops/sec to about 76k ops/sec. 
For executor.ts, I’d split the work into three passes.
First pass: cheap wins with low risk.
The easiest OTel-style fix is the emptiness check in handleResp(): Object.keys(executionContext.data).length > 0. That allocates an array just to answer “is this empty?”. OTel explicitly removed this kind of pattern in hot code. Replace it with a tiny hasOwnProperties() helper using for...in, or even better, track a boolean in the execution context the first time data is written. 
Also in this bucket: Array.from(representationsByVariableName.values()).some(...) is the same class of problem. It materializes an array only to scan it once. A plain for...of over .values() with early return is better. Likewise, pathSegments.filter(...).map(...) for errorPath is classic “pretty but expensive” hot-path code; OTel’s PRs are basically a manifesto against this style in perf-sensitive paths. 
Second pass: remove recurring allocation churn.
traverseFlattenPath() is a much bigger deal than it looks. It does const [segment, ...rest] = remainingPath; on every recursive step, which means a fresh array allocation every time you descend. On large responses with nested lists, that’s exactly the kind of death-by-a-thousand-cuts OTel was removing with Object.entries and Object.keys(...).length cleanup. Rewrite this function to take pathIndex instead of remainingPath, and use const segment = pathSegments[pathIndex]. Same behavior, way less garbage. 
Related to that, collectFlattenEntities() does path.slice() every time it records an entity path. Some copying is unavoidable because you need a stable snapshot, but I’d still profile this carefully because it sits directly on top of flatten trav...
This pull request was created from Copilot chat.
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.