Skip to content

fix: improve performance of orderSequenceByDependencies - #3216

Closed
ysmolski wants to merge 3 commits into
mainfrom
yury/router-652-router-high-cpu-on-queries-with-50-selections
Closed

ysmolski wants to merge 3 commits into
mainfrom
yury/router-652-router-high-cpu-on-queries-with-50-selections

Conversation

@ysmolski

@ysmolski ysmolski commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

There are multiple improvements that I have applied to this post processor.
This happened on plans created for mutations with more than 30 fetches.
Such plans have densely populated dependencies
(each next fetch in a sequence depends on all fetches before).

In the end that post-processor was completely rewritten.
Things were done roughly in this order:

  • I have pre-computed the calculation of all dependencies per fetch.
  • Topological comparisons were simplified because the size of
    dependencies gives the good ordering, that is only followed by the fetch IDs.
  • In the pre-compute instead of using maps I drafted a simple implementation
    of bitset since fetch IDs are densely populated.

Summary by CodeRabbit

  • Chores

    • Updated GraphQL tooling used by the router and router test components to a newer stable release, improving consistency across related components.
  • Documentation

    • Corrected test documentation describing cases where deferred GraphQL results may be discarded because matching non-deferred fields are already present.

There are multiple improvement that I have applied to this post
processor. This happened on plans created for mutation with more than 30
fetches. Such plans have densely populated dependencies (each next fetch
in a sequence depends on all fetches before).

In the end that post-processor was completely rewritten.
Things were done roughly in this order:

 * I have pre-computed the calculation of all dependencies per fetch.
 * Topological comparisons were simplified because the size of
   dependencies gives the good ordering, that is only followed by the
   fetch IDs.
 * In the pre-compute instead of using maps I drafted very simple
   implementation of bitset since fetch IDs are densely populated.
@ysmolski
ysmolski requested a review from a team as a code owner September 7, 2026 09:42

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

Tip: disable this comment in your organization's Code Review settings.

@github-actions github-actions Bot added the router label Sep 7, 2026
@coderabbitai

coderabbitai Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 51acda45-7bd6-471b-8832-c2adc22358f2

📥 Commits

Reviewing files that changed from the base of the PR and between 7e252b3 and 4404f10.

⛔ Files ignored due to path filters (2)
  • router-tests/go.sum is excluded by !**/*.sum
  • router/go.sum is excluded by !**/*.sum
📒 Files selected for processing (2)
  • router-tests/go.mod
  • router/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
  • router-tests/go.mod

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


Walkthrough

The PR updates the GraphQL tools dependency in both router modules to v2.20.1 and corrects a comment describing @defer behavior.

Changes

Router dependency and defer test maintenance

Layer / File(s) Summary
GraphQL tools dependency update
router/go.mod, router-tests/go.mod
Both modules update github.com/wundergraph/graphql-go-tools/v2 to release v2.20.1.
Defer test comment correction
router-tests/protocol/defer_test.go
The comment describing when @defer can be discarded is corrected.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 4404f

This updates the GraphQL tools release used by the router modules and corrects @defer test guidance. No current merge-blocking behavior or integrity risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main performance improvement to orderSequenceByDependencies. It does not include an issue identifier. At 55 characters, it exceeds the preferred 50-character length.
Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (2 skipped: 2 unsupported.)

  • Fix all pre-merge checks with AI

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

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

Router image scan passed

✅ No security vulnerabilities found in image:

ghcr.io/wundergraph/cosmo/router:sha-c1d3153884677556f3f87277c361a08807852bbf

@codecov

codecov Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 62.99%. Comparing base (cfcefd3) to head (4404f10).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3216      +/-   ##
==========================================
- Coverage   66.99%   62.99%   -4.00%     
==========================================
  Files         778      266     -512     
  Lines       63328    31575   -31753     
  Branches     7847        0    -7847     
==========================================
- Hits        42425    19890   -22535     
+ Misses      18193    10144    -8049     
+ Partials     2710     1541    -1169     

see 515 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@endigma endigma closed this in #3217 Sep 8, 2026
@ysmolski
ysmolski deleted the yury/router-652-router-high-cpu-on-queries-with-50-selections branch September 8, 2026 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant