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

Initialize the project cache more selectively #2608

Merged
merged 2 commits into from
Oct 10, 2022
Merged

Conversation

acao
Copy link
Member

@acao acao commented Jul 31, 2022

Massive optimization for multi-project users!

Warning: this changes the cache lifecycle a lot, especially after config change. Might need some fine tuning 😆
I should add lots of tests actually.

Changes

WorkspaceMessageProcessor._updateGraphQLConfig() becomes _loadAndCacheProjectConfig(projectName: string), which has grown up and become more discerning and instead of re-building all project config caches, we just load the project config it needs to handle that event, of course! What a poor design of mine before.

Because of this, the cache invalidation strategy on configuration and settings changes must change, and also allow project caches to lazily load as needed.

TODO:

  • as part of this, we must get rid of this._isInitialized(), should be detected per-project. otherwise one project cache that had an error initializing (we perform parsing and load files, etc) will prevent all other projects from having a cache or language features.
  • figure out what to do on MessageProcessor.handleDidChangeConfiguration - perhaps re-create the cache reset behaviour using a public method for WorkspaceMessageProcessor?
  • will building the project cache lazily on editor open or save events (not workspace) make working with the language? perhaps we need to add per-project cache initialization to onChange as well. again, this fires only once per project config per server init or config file or settings change events.
  • make package.json change events ignored more quickly via a quick parse-and-check for the top-level graphql namespace.
  • on workspace file changes, gently update just the cache for that file (maybe seperate PR)

@changeset-bot
Copy link

changeset-bot bot commented Jul 31, 2022

⚠️ No Changeset found

Latest commit: b3b5f48

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@acao acao added the lsp-server graphql-language-service-server label Jul 31, 2022
@github-actions
Copy link
Contributor

The latest changes of this PR are not available as canary, since there are no linked changesets for this PR.

@acao
Copy link
Member Author

acao commented Jul 31, 2022

actually, I think i have to get rid of this.isInitialized() entirely!

@acao acao marked this pull request as draft July 31, 2022 09:14
@Foo-x
Copy link
Contributor

Foo-x commented Jul 31, 2022

This looks great and the TODO is very clear!

@acao
Copy link
Member Author

acao commented Aug 6, 2022

@Foo-x getting back to it today!

@acao acao changed the base branch from main to revert-2612-revert-multi-root August 6, 2022 18:59
@acao acao force-pushed the lsp-project-init-optimization branch from fe04655 to 802434f Compare August 7, 2022 10:59
@codecov
Copy link

codecov bot commented Aug 7, 2022

Codecov Report

Base: 65.70% // Head: 69.15% // Increases project coverage by +3.44% 🎉

Coverage data is based on head (802434f) compared to base (2d91916).
Patch coverage: 23.84% of modified lines in pull request are covered.

❗ Current head 802434f differs from pull request most recent head b3b5f48. Consider uploading reports for the commit b3b5f48 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2608      +/-   ##
==========================================
+ Coverage   65.70%   69.15%   +3.44%     
==========================================
  Files          85       72      -13     
  Lines        5106     4295     -811     
  Branches     1631     1439     -192     
==========================================
- Hits         3355     2970     -385     
+ Misses       1747     1320     -427     
- Partials        4        5       +1     
Impacted Files Coverage Δ
packages/codemirror-graphql/src/lint.ts 100.00% <ø> (ø)
packages/codemirror-graphql/src/results/mode.ts 47.05% <ø> (ø)
packages/codemirror-graphql/src/utils/hintList.ts 95.65% <ø> (ø)
...ckages/codemirror-graphql/src/utils/mode-indent.ts 0.00% <0.00%> (ø)
packages/codemirror-graphql/src/variables/mode.ts 79.48% <ø> (ø)
packages/graphiql-react/src/editor/whitespace.ts 100.00% <ø> (ø)
packages/graphiql-react/src/utility/debounce.ts 0.00% <0.00%> (ø)
packages/graphiql-react/src/editor/tabs.ts 5.66% <5.66%> (ø)
packages/codemirror-graphql/src/variables/lint.ts 47.61% <66.66%> (+0.63%) ⬆️
packages/codemirror-graphql/src/hint.ts 94.73% <100.00%> (ø)
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@acao acao marked this pull request as ready for review August 7, 2022 11:30
@acao
Copy link
Member Author

acao commented Aug 7, 2022

ok! now it's ready for review 🥳

now that we (re) initialize the project caches more selectively in this PR, i much feel better about multi-workspace support
we even ensure the project is cached on workspace changes, and on change, not just on did open or save

i also feel good about the config change events. we invalidate the init status so the project caches will lazily rebuild on one of many events

@acao acao force-pushed the lsp-project-init-optimization branch from 802434f to 3971313 Compare August 7, 2022 14:29
@acao acao force-pushed the revert-2612-revert-multi-root branch from 73d6f68 to 9306f66 Compare October 8, 2022 23:09
Base automatically changed from revert-2612-revert-multi-root to main October 10, 2022 19:34
@acao acao force-pushed the lsp-project-init-optimization branch 2 times, most recently from 231bc45 to e791427 Compare October 10, 2022 20:12
Massive optimization for multi-project users!

fix up tests a bit
@acao acao force-pushed the lsp-project-init-optimization branch from e791427 to 62bb480 Compare October 10, 2022 20:44
@acao acao merged commit a753c33 into main Oct 10, 2022
@acao acao deleted the lsp-project-init-optimization branch October 10, 2022 21:26
acao added a commit that referenced this pull request Oct 10, 2022
acao added a commit that referenced this pull request Oct 10, 2022
acao added a commit that referenced this pull request Oct 10, 2022
acao added a commit that referenced this pull request Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp-server graphql-language-service-server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants