perf(import): remove redundant work in addDefinition's dependency traversal#8259
perf(import): remove redundant work in addDefinition's dependency traversal#8259lemonmade wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 5a7277c The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesaddDefinition traversal optimization
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
lemonmade
force-pushed
the
perf/import-faster-addDefinition
branch
from
9d95a37 to
c4414d1
Compare
|
Added JSDoc for |
|
@coderabbitai review |
✅ Action performedReview finished.
|
…inition addDefinition recurses across the whole dependency graph. Two things were recomputed on every call: the invariant visitedFiles.get(filePath), and each field's dependency-name set (a pure function of the field node). Hoist the former and memoize the latter by field identity. Pure performance change — output is byte-identical (80 import tests pass; a real ~12k-line schema produces identical results). On top of the print() memoization this took a real codegen pass from ~17s to ~13s. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lemonmade
force-pushed
the
perf/import-faster-addDefinition
branch
from
c4414d1 to
5a7277c
Compare
|
@coderabbitai The Docstring Coverage check reports 0.00% even though the two functions this PR modifies — |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 8259 --repo ardatan/graphql-tools --json statusCheckRollup --jq '.statusCheckRollup[] | {name: .name, workflowName: .workflowName, conclusion: .conclusion, detailsUrl: .detailsUrl}'Length of output: 7225 🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n "docstring" -i . --hidden -g '!node_modules' -g '!.git' | head -100Length of output: 567 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 8259 --repo ardatan/graphql-tools --json comments --jq '.comments[] | select(.body | test("(?i)docstring")) | {author: .author.login, body: .body}' | head -c 4000
echo "---REVIEWS---"
gh api repos/ardatan/graphql-tools/pulls/8259/reviews --jq '.[] | select(.body | test("(?i)docstring")) | {user: .user.login, body: .body}' | head -c 4000Length of output: 4657 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 8259 --repo ardatan/graphql-tools --json comments --jq '.comments[] | select(.body | test("(?i)docstring")) | .body' > /tmp/cr_comment.txt
wc -l /tmp/cr_comment.txt
sed -n '1,400p' /tmp/cr_comment.txtLength of output: 7854 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr diff 8259 --repo ardatan/graphql-tools > /tmp/pr.diff
wc -l /tmp/pr.diff
grep -n "^+function\|^+export function\|^+const .*= (\|^+ function\|=>" /tmp/pr.diff | head -80Length of output: 457 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat -n /tmp/pr.diffLength of output: 6336 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api repos/ardatan/graphql-tools/pulls/8259/commits --jq '.[] | {sha: .sha[0:12], date: .commit.author.date, message: .commit.message}'
echo "---"
gh pr view 8259 --repo ardatan/graphql-tools --json comments --jq '.comments[] | {author: .author.login, createdAt: .createdAt, snippet: (.body[0:80])}'Length of output: 2072
Here's what happened, based on the PR history:
To get an up-to-date Docstring Coverage number, please ask for a full review rather than the incremental one — that forces recomputation of the pre-merge checks table. I've triggered that now. ✅ Action performedFull review finished. |
1 participant
Problem
When
processImportassembles a definition's imported dependencies, the inneraddDefinitionclosure recurses across the entire dependency graph. Two things were recomputed on every recursive call:const fileDefinitionMap = visitedFiles.get(filePath)—filePathis invariant for the lifetime of thevisitFilecall, so thisMap.get(over a long absolute-path key) was repeated across the whole — potentially very large — recursion.Mapwas built and the field re-visited (visitFieldDefinitionNode/visitInputValueDefinitionNode) to collect dependency names. That result is a pure function of the field node and the file's staticdependenciesByDefinitionName, but it was recomputed every time the owning definition was added to any set.On a large, densely-connected schema these two lines dominate what remains of import time after the
printmemoization in #8258 (CPU profiling attributed ~85% of the post-#8258 time toaddDefinitionand its field loop).Fix
visitedFiles.get(filePath)lookup out ofaddDefinition.Both are pure performance changes — no algorithmic/output change.
Validation
@graphql-tools/importtests pass.printmemoization), the same downstream codegen pass went from ~133s → ~13s. This PR contributes the ~17s → ~13s step on top of perf(import): memoize print() in processImport (O(n²) → O(n) on large schemas) #8258. (It composes with perf(import): memoize print() in processImport (O(n²) → O(n) on large schemas) #8258 but is independent — they touch different functions.)Not addressed here
The deeper cost is structural: each definition's transitive closure is still rebuilt independently (an O(n²) traversal). Reducing that needs a shared closure cache and is more invasive, so I've left it out of this PR to keep it small and obviously-safe. Happy to follow up if there's interest.
🤖 Generated with Claude Code