[jhonDoe15]

Description

Completing a synchronous scalar/enum list currently allocates an indexed path object for every successful item, even though scalar serialization receives no indexed resolve info. Resolve the nullable leaf type once and create a stable indexed path only for failures or asynchronous completion.

The same serializer still runs once per item. completeListItemValue owns one shared completion/error/async pipeline; it materializes the indexed path only when fallback completion, errors or async work require it. Nulls, Errors, promised inputs, composite types, active streams and async iterators retain their existing behavior. Tests cover custom coercion and its ordering, nullable error indexes, asynchronous serialization/rejections, non-null bubbling, iterator cleanup and thenable getter reads.

Related #8616.

Type of change

• Non-breaking performance improvement; patch changeset for @graphql-tools/executor.

How Has This Been Tested?

• Repository build, npm run ts:check and changed-source ESLint pass.
• Full Jest suite: 113 suites / 1,765 tests pass, two existing skipped tests and 11 passing snapshots. A prior overlapping local run collided on the URLLoader fixture's port 8081; the exclusive full rerun passed without source/test changes or skips.
• The existing incremental-delivery and cancellation suites remain passing. Additional characterization pins asynchronous non-null serializer failure indexes and the baseline's two thenable getter reads. The latter caught and corrected an extra classification read during this refactor.
• The two-million-Float workload now extends the existing execution/__benchmarks__/executor.bench.ts Vitest suite and CI comparison flow. Earlier standalone timing numbers described the pre-review implementation and have been removed; revised measurements are reported with their own validation evidence.
• Five alternating fresh-process comparisons against the same built master, two million Float values: median CPU 81.82 → 41.89 ms, elapsed 70.00 → 32.84 ms; serialized hashes match. The shared Vitest workload measured 360.23 → 115.81 ms mean in separate current/base runs. These are isolated workload observations, not general gateway latency guarantees.
• Combining the actual revised mainline sources with perf(executor): reduce retained storage for small list results #8617 passes build/typecheck and all 1,771 tests, two existing skips. Five fresh paired comparisons also preserve output hashes for matrix widths 2/8/16/1000, the large Float vector and repeated 1000-string lists. This evidence does not use the old 1.x backport or attribute earlier gateway numbers to the new combination.

Reproduce after building:

npm run test:bench -- executor.bench.ts

Use BENCH_PACKAGE_ROOT to point the same suite at a separately built baseline checkout, as CI does. Test environment: Node 24.15.0, Linux ARM64, GraphQL 17.0.2. Local full-suite browser tests used Chromium and IPv4-first DNS for localhost servers.

CI remediation

The shared baseline leak failure was traced to Babel's cached template builder retaining a raw origin Error and V8 call-site receivers, including the calling Jest VM. The independent fix #8621, included here, uses the existing patch-package postinstall to retain only a formatted origin-stack string. Seven lifecycle/diagnostic checks cover collection, explicit-retention controls, usable builders and custom stack formatting. No detectors, retries, timeouts or existing suppression rules changed. All four Node22/24 × GraphQL16/17 CI jobs pass on the companion; this branch's new exact-head run is assessed separately. The final local build and all 1,765 ordinary tests pass with the patch applied.

Exact head 73436ed942dd36cc154d51b6a7f5ed28ac6ba410: CI run 40588414792 passes all four Node22/24 × GraphQL16/17 jobs, including leak checks, along with build/types/lint and Bun. CodeQL, analysis and benchmark comparison also pass.

Compatibility and discussion

Targets the active master line, currently executor 4.1.6, and requires no new dependency or engine version. It is independent of the completed-list storage patch, although both touch completeListValue and should be integrated with normal merge coordination.

The maintainer confirmed that older executor majors do not receive maintained backport releases. Supported upstream adoption uses the current executor through Yoga 6 and matching Hive versions. Yoga 5's executor 1.x range does not receive this mainline change.

The older 1.5.7 source reference is unofficial and preserves the initial proposal, not these subsequent review revisions. It is not a released package, supported maintenance line or recommendation for production replacement. Do not force executor 4.x underneath Yoga 5.

Checklist

• Followed the contribution/style guidance and opened the related issue first.
• Performed a self-review and commented the path-allocation rationale.
• Added regression tests and a generic reproduction; no new source lint warnings.
• Existing and new tests, typecheck and build pass locally.
• No dependent package/API changes are required for this mainline patch.

Developed and validated with Codex (GPT-6.1); the included reproduction supplies the performance evidence.

[coderabbitai]

No actionable comments were generated in the recent review.

Recent review info

Run configuration

• Source mode: Auto
• Repository: Unavailable
• Branch: Unavailable
• File: No configuration file from this repository applied
• Commit: Unavailable
• Layers: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: c7fec5d4-63a0-462d-8dc6-a8b7e4eba5e7

Commits

Reviewing files that changed from the base of the PR and between cde9563 and 73436ed.

Files selected for processing: 4

• .github/workflows/tests.yml
• package.json
• patches/@babel+template+8.0.0.patch
• scripts/test-babel-template-retention.mjs

Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 5 remain after this review.

📝 Summary

Summary by CodeRabbit

• Performance
  • Improved execution efficiency for synchronous lists of scalar and enum values. Item errors continue to report indexed locations, and asynchronous completion and incremental delivery remain unchanged.
• Bug Fixes
  • Improved error stack reporting for template failures, including when stack formatting is customized or throws. Template errors retain their original cause.

For non-streamed lists with leaf item types, the executor avoids creating indexed paths before serializing eligible synchronous items. The pull request adds tests and a benchmark for this behavior. It also adds Babel template retention tests and changes origin-stack capture.

Changes: Scalar list execution

Layer / File(s)	Summary
Defer indexed paths for leaf items packages/executor/src/execution/execute.ts, packages/executor/src/execution/__tests__/scalar-list-paths-test.ts, packages/executor/src/execution/__benchmarks__/executor.bench.ts, .changeset/lazy-scalar-list-item-paths.md	Eligible synchronous, non-null, non-Error leaf items are serialized without first creating indexed paths. Error handling retains item paths, while streamed lists and other item types keep their existing path behavior. Tests cover serialization, error bubbling, iterator closure, nullish and promised items, and thenable getter access. A dense Float list benchmark and patch changeset are added.

Babel template lifecycle

Layer / File(s)	Summary
Capture origin stacks and test template retention patches/@babel+template+8.0.0.patch, scripts/test-babel-template-retention.mjs, package.json, .github/workflows/tests.yml	The Babel patch appends an origin stack only when capture returns a string. New tests check garbage collection, builder usability, template errors, custom stack formatters, and preservation of substitution errors. The package script and workflow run these tests.

Priority: Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Possibly related issues

• ardatan/graphql-tools#8616: This pull request implements the described indexed-path optimization for successful synchronous scalar and enum list items.
• ardatan/graphql-tools#8619: The Babel patch and retention tests address the described template-origin stack retention during leak checks.

Possibly related PRs

• ardatan/graphql-tools#8617: This PR changes the same executor list-completion implementation and benchmark, and its result compaction uses the completion output changed here.
• ardatan/graphql-tools#8621: This PR contains the same Babel patch, lifecycle script, workflow step, and retention tests.

Suggested reviewers: ardatan

Merge Risk: Minimal · up to 73436

No actionable regression is established by the reviewed changes; the PR is mergeable after normal checks.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surv…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title is concise and accurately identifies the main executor performance change: allocating scalar list paths only when needed.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description check		The pull request description directly explains the scalar and enum list path-allocation optimization, affected behavior, tests, benchmarks, CI remediation, and compatibility scope. It matches the chan…

Full details: Description check

Explanation

The pull request description directly explains the scalar and enum list path-allocation optimization, affected behavior, tests, benchmarks, CI remediation, and compatibility scope. It matches the changeset.

Full details: Docstring Coverage

Explanation

Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 4 files. (3 skipped: 3 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each list item’s place
And leaves no path for success to trace
It guards the errors as they appear
While template stacks stay clear
Then bounds through tests with whiskers bright}

_{Comment @coderabbitai help to get the list of available commands.}

[changeset-bot]

🦋 Changeset detected

Latest commit: 73436ed

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@graphql-tools/executor	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ardatan]

Thanks for the careful leaf fast path and the characterization tests. Behavior looks aligned with the existing completeListItemValue path for the cases that matter (null / Error / Promise items, NonNull bubbling, async serialize, @stream left on the slow path).

Two asks before merge:

1. Structure: the success path currently duplicates the error / async handling from completeListItemValue. Could we keep a single completion path and allocate the indexed item path lazily (only when error or async completion needs it)? That would avoid drift between the two copies over time.
2. Benchmarks: we already have a Vitest bench suite for the executor (packages/executor/src/execution/__benchmarks__/executor.bench.ts, via npm run test:bench, compared against master in CI). Prefer extending that over the standalone scalar-list-paths.mjs, so the new case stays in the shared compare flow.

Not blocking on correctness from what I can see; happy to re-review after the cleanup / bench move.

[jhonDoe15]

The shared completion path and benchmark revisions are implemented in cde9563, following your review. The shared CI leak failure is addressed separately in #8621 and included here in 73436ed.

Changes

• One completion/error/async pipeline: completeListItemValue receives the parent path and stable item index, materializing the concrete path only for fallback completion, async work or errors. Synchronous scalar success adds no per-item lazy-path closure.
• Observable behavior pinned: promise classification is cached once. A new test caught and corrected an extra then getter read, restoring the original two reads. Another checks asynchronous non-null failure at its original aliased index. Stream/defer/cancellation and iterator-cleanup coverage remains passing.
• Shared benchmark: the two-million-Float workload now lives in the existing Vitest suite; the standalone script was removed.

Performance evidence

Two million synchronous Float values, Node 24.15.0 / Linux ARM64 / GraphQL 17.0.2:

Measurement	Built master	Revised
Fresh-process CPU, median	81.82 ms	41.89 ms
Fresh-process elapsed, median	70.00 ms	32.84 ms
Shared Vitest workload, mean	360.23 ms	115.81 ms

Fresh measurements use five alternating paired processes with matching serialized hashes. Vitest uses separate current/base runs. These are workload-specific observations, not general gateway latency claims.

Local validation

• Build, typecheck, source lint and formatting pass.
• 1,765 tests pass, two existing skips.
• The actual revised mainline combination with perf(executor): reduce retained storage for small list results #8617 passes 1,771 tests, build/typecheck and the combined Vitest suite. Earlier implementation/backport measurements are kept separate in the PR description.

CI remediation

Babel cached a template builder retaining a raw origin Error and its V8 call-site receivers, including a calling Jest VM. The existing patch-package workflow now retains a formatted stack string instead. Seven lifecycle/error-diagnostic tests cover collection, explicit retention, usable builders and custom stack formatters.

The companion's four Node 22/24 × GraphQL 16/17 CI jobs pass, including the existing leak checks. After applying that isolated development-tooling fix here, a fresh build and 1,765 tests pass locally.

All exact-head checks pass on 73436ed: run 40588414792 passes all four Node 22/24 × GraphQL 16/17 test jobs, including leak checks. Types/lint, Bun, CodeQL, analysis, benchmark comparison and automated review pass too. The maintainer's earlier review remains pending re-review.

Both leak detectors, retries, timeouts, existing skips and suppression rules remain unchanged.

Baseline evidence and its limits

The same custom-loader leak signature appears on unchanged master CI. A local Node 22.23.3 / GraphQL 17 comparison also reproduced it on unchanged base and revised list head. That local comparison used the list branch, not an exact reproduction of this scalar CI runner.

The minimal reproduction requires only a fresh module exporting 123; heap analysis traced the retained VM to Babel's origin Error. See #8619 and babel/babel#18436. One additional local Node24/GraphQL17 full run flagged error-handling.test.ts; three focused runs and its exact 52-suite prefix passed, as did all three PRs' exact-head CI matrices. That local failure remains recorded but unreproduced, with no identified retaining root or speculative fix.

Next

Please re-review the shared completion path. This PR remains independent on master, with sequential landing and merge coordination for the shared source/benchmark edits.

[ardatan]

Thanks for the revision. The shared completion path and Vitest bench look good, and the CI compare shows a clear win on the dense scalar-list cases. Merging.