Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClientSideBaseVisitor.fragmentsGraph is recomputed several times despite never changing #10018

Closed
vhfmag opened this issue Jun 21, 2024 · 2 comments

Comments

@vhfmag
Copy link
Contributor

vhfmag commented Jun 21, 2024

Which packages are impacted by your issue?

@graphql-codegen/visitor-plugin-common

Describe the bug

While profiling the graphql codegen step of one of my company's projects a sharp difference in build time after migrating from near-operation-file to client-preset, we identified that a disproportionate time is spent in these three lines:

const fragmentDependencyNames = new Set(
  fragmentNames.map(name => this.fragmentsGraph.dependenciesOf(name)).flatMap(item => item)
);

You can see that in the flamegraph below, where all of the green traces on the bottom are time spent on those lines — I added a couple arrows to help visualize which trace I mean, but it's all traces of the same color, not just these 4

Screenshot 2024-06-21 at 15 51 28

The reason why that line is so expensive is that this.fragmentsGraph is an expensive getter. In addition to that, it only depends on this._fragments and this._extractFragments(). The this._extractFragments only depends on this._fragments and itself (it's recursive), and this._fragments isn't reassigned or modified (which we verified by changing its declaration to private readonly _fragments: ReadonlyMap<string, LoadedFragment>).

With that in mind, we converted fragmentsGraph from a getter to a regular property, and our build time went from ~130 seconds to ~50s. I'm opening this issue to share our findings and share a PR proposing a related performance improvement.

Your Example Website or App

N/A

Steps to Reproduce the Bug or Issue

  1. Run codegen on a mid-sized project using client-preset and measure the build time
  2. Make the change to only compute fragmentsGraph once per ClientSideBaseVisitor instance
  3. Run codegen again and compare the build time

Expected behavior

client-preset should be faster by avoiding costly recomputations of unchanged data

Screenshots or Videos

No response

Platform

  • OS: macOS
  • NodeJS: 18.18.2
  • graphql version: 16.2.0
  • @graphql-codegen/client-preset version(s): 4.2.6
  • @graphql-codegen/visitor-plugin-common version(s): 2.13.1

Codegen Config File

No response

Additional context

No response

@vhfmag
Copy link
Contributor Author

vhfmag commented Jun 27, 2024

Hi @n1ru4l! I see you merged #10019 (thanks!), but there hasn't been a version release yet. I'm wondering what the usual release process is

@n1ru4l
Copy link
Collaborator

n1ru4l commented Jun 28, 2024

@vhfmag Releasing it right now 😇 #10010

@n1ru4l n1ru4l closed this as completed Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants