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

Load DSL compilers and extensions ahead of time in add-on mode #2152

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jan 14, 2025

Motivation

While profiling DSL generation in the add-on, I noticed we were spending a lot of time running Dir.glob. The code path that lead to invoking glob multiple times was loading DSL compilers and extensions, which invokes both glob directly and Gem.find_files (which invokes glob under the hood).

There's no point in reloading DSL compilers and extensions every single time. We can load it once and then the forked process will have all of the expected classes loaded already.

Implementation

We need to use the workspace path declared by the editor to load all of this (which may not match Dir.pwd), so I made a new notification in the server add-on. It gets invoked once after boot.

The other side of the change is to stop loading the compilers and extensions in the regular DSL path, which how we can save some time.

@vinistock vinistock added the refactor Code refactor with no behaviour change label Jan 14, 2025 — with Graphite App
Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock marked this pull request as ready for review January 14, 2025 14:59
@vinistock vinistock requested a review from a team as a code owner January 14, 2025 14:59
@paracycle
Copy link
Member

Do we need this extra complexity? Can we not make the loader be idempotent and refuse to do any loading if called a second time? That wouldn't require any knowledge about LSP mode or anything.

@vinistock vinistock force-pushed the 01-14-load_dsl_compilers_and_extensions_ahead_of_time_in_add-on_mode branch from e61dae2 to 41aaa28 Compare January 14, 2025 15:32
@vinistock
Copy link
Member Author

Do we need this extra complexity? Can we not make the loader be idempotent and refuse to do any loading if called a second time? That wouldn't require any knowledge about LSP mode or anything.

I indeed would prefer that approach, but there are several layers of why that's not possible with the current approach. Instead of decoupling the CLI from the core of Tapioca, so that we could have two frontends (CLI + add-on), we are invoking the CLI by reloading it.

That approach was causing severe memory bloat because loading the CLI multiple times is not in any way idempotent. To move forward with the prototype, we started running DSL in a fork to avoid polluting the memory of the main process - which fixes the bloat, but also means that we lose all of the state between runs.

A simple thing like this wouldn't work because the separate runs won't share state.

def load
  return if @loaded

  # ...

  @loaded = true
end

@vinistock vinistock force-pushed the 01-14-load_dsl_compilers_and_extensions_ahead_of_time_in_add-on_mode branch from 41aaa28 to b298065 Compare January 14, 2025 16:03
@vinistock vinistock merged commit 85bf9f2 into main Jan 15, 2025
35 checks passed
@vinistock vinistock deleted the 01-14-load_dsl_compilers_and_extensions_ahead_of_time_in_add-on_mode branch January 15, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore refactor Code refactor with no behaviour change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants