solargraph icon indicating copy to clipboard operation
solargraph copied to clipboard

Improve pin caching

Open apiology opened this issue 6 months ago • 2 comments

This PR has been broken into the following parts:

  • https://github.com/castwide/solargraph/issues/1042
  • https://github.com/castwide/solargraph/pull/1059
  • https://github.com/castwide/solargraph/pull/1057
  • https://github.com/castwide/solargraph/pull/1060
  • #1061
  • #1067
  • #1064
  • https://github.com/castwide/solargraph/pull/1112
  • https://github.com/castwide/solargraph/pull/1113

I'll keep this open and use it after these are merged to make sure the result is what I was looking for, but you can disregard this diff entirely.

Original PR description:

Way too much inside - apologies for the large monolithic change:

  1. solargraph gems caches all cacheable gems/core/stdlibs for a workspace, allowing for CI-based ahead-of-time caching
  2. Rebuild logic to deal with external bundles and add specs
    • Interested in whether this helps real-world scenarios!
    • Big caveat: all gems need to be available in the environment Solargraph runs in - maybe we could dynamically install them in the future?
  3. Performance: Define what we need from Parser gem in RBS and stop trying to process it as YARD (one-off for now, but can be made into config preferences for other projects...)
  4. Refactor things out of DocMap class:
    • Workspace::Gemspecs: Resolves gemspecs and dependencies from bundle
    • Workspace::RequirePaths: Calculates the require paths from config and .gemspec file if it exists
    • PinCache: Gem building logic
  5. 100% spec coverage of all affected lines, including more than a few new scenarios
  6. Improved resolution of requires into gems driven by specific problematic cases now in specs
  7. More linting (probably too much) to make RuboCop happy
  8. Add rspec timeouts, which were valuable figuring out some plugin-related issues in the LSP specs.

Perf note

Watching the checks, I was a little worried the plugin regression workflows had slowed down. After isolating things a little more, I think it's possible, but I'm not convinced that more data points won't average this all down to the same thing in the end. I did separate out the plugin regression workflows to tease out impact of each. Here's the data I see:

Specs:

  • solargraph-rails alone: ~5m
  • solargraph-rspec alone: ~7m
  • both plugins: ~13m
  • no plugins specs alone: ~7m

Typechecking

  • solargraph-rails alone: ~2.5m
  • solargraph-rspec alone: ~2.5m
  • both plugins: ~3m
  • no plugins: ~2.5m

I'd suggest we keep an eye on this, but I am comfortable personally moving forward with this PR in the meantime.

PR complexity note

I'm working now on splitting this into smaller PRs - as those are merged I hope to get this down small enough to be reviewed more easily.

Progress so far:

  • 2025-08-24: 62 changed files (starting place)

Next steps:

  • [X] Merge master back in after recent PRs
  • [ ] Reproduce "Completion requests that previously took a fraction of a second now take up to 6 seconds." and write spec if feasible
  • [ ] Reproduce "Go-to-definition no longer works on gem sources." and write spec if feasible
  • [ ] Fix "Completion requests that previously took a fraction of a second now take up to 6 seconds."
  • [ ] Fix "Go-to-definition no longer works on gem sources."
  • [ ] Get CI green again
  • [ ] Merge #1039
  • [ ] Squash commits

apiology avatar Jul 15 '25 18:07 apiology

The sheer size of this PR makes code review difficult. Testing locally, however, I found two significant issues:

  • Completion requests that previously took a fraction of a second now take up to 6 seconds.
  • Go-to-definition no longer works on gem sources.

We might need to break these changes into smaller PRs to mitigate regression bugs.

castwide avatar Aug 24 '25 14:08 castwide

  • Completion requests that previously took a fraction of a second now take up to 6 seconds.
  • Go-to-definition no longer works on gem sources.

@castwide: I fixed a big perf regression in the last commit (and in the extracted version in #1064). I verified I could go-to-definition on a gem source - would you be so kind as to retest to make sure there was nothing else causing those problems?

apiology avatar Sep 09 '25 15:09 apiology