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

Limit ftplugin to buffer and avoid early lua requires #332

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Slotos
Copy link

@Slotos Slotos commented Mar 4, 2025

This changes ftplugin behaviour to:

  • create user commands confined to the buffer it was triggered on
  • avoid filesystem lookups of require (they add up) by postponing
    them until user command gets invoked
  • skip loading if it has already been executed in a buffer

To deduplicate ftplugins, lua's dofile is used to load a specific
file that holds all shared commands. This is equivalent to vimscript
standard source approach.

These make ftplugin portion of this plugin behave like a good neighbor
ftplugin - its changes are buffer local, as it's invoked when a buffer
filetype matches.

Additionally, ftplugins now read global disable_typescript_tools
variable, returning early if it's set. Not a common behaviour for
ftplugins in general, but a good way to give users fine control over
plugin behaviour nonetheless.

Not tackled is cleanup in case buffer filetype changes without reloading.
I don't think routines for that currently exists, in fact.

Additional housekeeping

  • ci: use actions/cache@v3 in tests workflow

  • fix: use non-deprecated iter_matches with all=true

    This fixes nightly test failures. This also changes behaviour in complex
    scenarios most likely, as instead of matching just the last node, we now
    consider all nodes in the match.

Slotos added 3 commits March 5, 2025 08:33
This changes ftplugin behaviour to:
- create user commands confined to the buffer it was triggered on
- avoid filesystem lookups of require (they add up) by postponing
  them until user command gets invoked
- skip loading if it has already been executed in a buffer

To deduplicate ftplugins, lua's `dofile` is used to load a specific
file that holds all shared commands. This is equivalent to vimscript
standard `source` approach.

These make ftplugin portion of this plugin behave like a good neighbor
ftplugin - its changes are buffer local, as it's invoked when a buffer
filetype matches.

Additionally, ftplugins now read global `disable_typescript_tools`
variable, returning early if it's set. Not a common behaviour for
ftplugins in general, but a good way to give users fine control over
plugin behaviour nonetheless.

Not tackled is cleanup in case buffer filetype changes without reloading.
I don't think routines for that currently exists, in fact.
This fixes nightly test failures. This also changes behaviour in complex
scenarios most likely, as instead of matching just the last node, we now
consider all nodes in the match.
@Slotos Slotos force-pushed the skip-filesystem-lookups-and-stay-in-buffer-ftplugin branch from 35e5ce5 to 2fe5303 Compare March 5, 2025 07:34
@shofel
Copy link

shofel commented Mar 19, 2025

@Slotos Can you please elaborate on the reason why dofile is preferable over require here?

As I see, only

avoid filesystem lookups of require

is attributed to dofile. The rest goodness comes independent of dofile.

And also:

  • require will search the path only once
  • then the module will be cached. And require saves up on parsing
    from :help require:

The function starts by looking into the package.loaded table to determine whether {modname} is already loaded. If it is, then require returns the value stored atpackage.loaded[modname].

Disclaimer. I can`t make this merging faster. I'm just curious :)

@Slotos
Copy link
Author

Slotos commented Mar 19, 2025

On the first call for each unique argument, require performs a lookup across a set of directories. This in itself can add up if multiple plugins use require eagerly. However, every plugin adds a lookup directory to the list, increasing average cost of initial requires. Postponing requires until the user command is invoked spreads this impact, and avoids it entirely if user doesn’t use true commands that need to require some code.

dofile loads a specific file, avoiding filesystem lookup cost.

But, your quote on require cache shows that I missed an easy and quite important optimization - the file only needs to be loaded once per ftplugin. I should cache the loads.

I’ll push an update in a few hours.

This eliminates repeated reads of ftplugin implementation file on
repeated filetype events. The only reliably persistent store available
to ftplugin scripts that I can think of is the global table, which is
why I'm using it to store loaded ftplugin "object" in. The prefix is a
string gneerated by rolling a fair dice in order to make name collision
highly improbable.

There might be a more efficient way to do this. Please tear this commit
apart if you know of it.
@Slotos Slotos force-pushed the skip-filesystem-lookups-and-stay-in-buffer-ftplugin branch from dd9c891 to 600943f Compare March 24, 2025 20:57
@shofel
Copy link

shofel commented Mar 28, 2025

However, every plugin adds a lookup directory to the list, increasing average cost of initial requires

@Slotos Every plugin adds a directory to lookup, I agree. But isn't it just by the existence of the directory in packpath, and irrelated to any calls to require?

So, by adding a cache you made the solution even closer to require, and increased complexity. But what is the upside? How much and how exactly it'll be worse to use require here? 🤔

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

Successfully merging this pull request may close these issues.

2 participants