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

fix(lazy): modify lazy events to improve perf #734

Closed
wants to merge 2 commits into from
Closed

fix(lazy): modify lazy events to improve perf #734

wants to merge 2 commits into from

Conversation

vollowx
Copy link
Contributor

@vollowx vollowx commented May 13, 2023

The current lazy events for Comment, vim-illuminate and treesitter are so early that actually they do not do anything at that time.

This PR changed the lazy events to BufReadPost and BufNewFile to let them load when there is a new buffer.

Before: about 70ms

After (bigfile enabled):

image

After (bigfile disabled):

image

* bigfile requires treesitter

@Jint-lzxy
Copy link
Collaborator

In fact, these plugins are "loaded on-demand". See #502 and :h CursorHold for more information. Note that external measuring tools are inaccurate at this time.

@vollowx
Copy link
Contributor Author

vollowx commented May 13, 2023

Yes, but these plugins only is needed in code files and other text buffers. I think it may be better load in buffer events. 🤔

@Jint-lzxy
Copy link
Collaborator

Yes, but these plugins only is needed in code files and other text buffers. I think it may be better load in buffer events. 🤔

That's true, but it will significantly increase the "user-perceived" startup time, especially in the case of directly opening a file for editing (b/c many events are called simultaneously). { "CursorHold", "CursorHoldI" } separates the startup of some plugins from the main event loop.

@vollowx
Copy link
Contributor Author

vollowx commented May 13, 2023

Got it

@vollowx vollowx closed this May 13, 2023
@Saafo
Copy link
Contributor

Saafo commented May 15, 2023

I'm still wonder why the bigfile.nvim cannot be lazy loaded, which do cost a lot of time when startup. Can we put it into buffer event, cursor event or VeryLazy?

@Jint-lzxy
Copy link
Collaborator

@Saafo b/c bigfile.nvim must be loaded before BufReadPre is called. What alternatives are u considering?

@Saafo
Copy link
Contributor

Saafo commented May 16, 2023

b/c bigfile.nvim must be loaded before BufReadPre is called. What alternatives are u considering?

I'm trying BufReadPre for directly open a big file like vim bigfile.lua, and BufWinEnter for auto restored bigfile by auto-session on launch.

The former seems work fine, but the later is quite odd. I order the time line in Lazy Profile, and found the BufWinEnter is before BufReadPre, and we register the callback here, but we can't receive the BufReadPre call back. (I add print "readd!!" locally like this:)

image

(but it seems work fine? since the highlight is disabled and the LspInfo shows no client attach to this buffer, and if we change lazy = false, still no "readd!!" print)

@Jint-lzxy
Copy link
Collaborator

@Saafo This might be a bit difficult to explain, but let me clarify that:

The root cause of the problems u encounter is you can't control the order of invocation of different callbacks registered under the same event during resource competition (obviously we do not have anything like mutexes). Yes, sometimes things work just b/c bigfile's callback is registered before its dependencies are loaded.

btw, Lazy Profile is not that accurate. It only records the order in which events are called during the current startup. Due to the very short interval between different events being called in your case and the fact that their order is recorded based upon real CPU time, this could change from startup to startup. We do NOT have strong guarantee that bigfile will always load before its dependencies, so there is no reason to adopt a configuration that "probably works", just b/c it can save startup time.

@Saafo
Copy link
Contributor

Saafo commented May 16, 2023

Got it! Thanks very much for the detailed explanation!

@vollowx vollowx deleted the fix/lazy branch June 17, 2023 04:13
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.

3 participants