stevearc / oil.nvim

Neovim file explorer: edit your filesystem like a buffer
MIT License
3.84k stars 110 forks source link

perf: only execute on current buffer since this event is called on each buffer #246

Closed mehalter closed 10 months ago

mehalter commented 10 months ago

When saving sessions, the file that you source (i.e. Session.vim) executes doautoall SessionLoadPost which will execute the autocmd event SessionLoadPost on each buffer. So we don't need to manually loop over each buffer ourselves, the autocmd execution actually handles it for us. This causes an exponential growth in the number of times these calculations are made.

Also I found this while investigating #245 but sadly this doesn't fix the problem root there.

EDIT: ~I just pushed another performance improvement to check if oil is already loaded by checking the filetype. When restoring a session with source like the original issue #29 the filetype is not set to oil, but when loading views with loadview which also calls SessionLoadPost the oil buffers are already loaded and therefore the filetype is already oil. This allows us to optimize this out. There might be a better way to check if oil is already loaded in a buffer, if so let me know! Another thing is that with this performance update it circumvents the problem in #245 so that it never happens, but doesn't fix the root issue which seems to be a bug in the load_oil_buffer function where even when it's only called on buffer numbers that belong to oil buffers, they sometimes clobber details in other buffer numbers which I believe is coming from the fact that those buffers do not actively belong to a window. This should still be investigated by someone more familiar with the codebase. Because this doesn't actually fix the bug in that function, it might not be considered "closing" of #245 even though the symptoms of the problem go away~

EDIT 2: Most up to date information on what this PR does and what it does not do can be found below: https://github.com/stevearc/oil.nvim/pull/246#issuecomment-1841370744

With the most recent changes we can also say this this resolves #245 since it should avoid the issue from ever happening, but it might come up down the road if someone gets into some complicated edge cases when using mksession. Chances are this is never going to happen though.

mehalter commented 10 months ago

That change does work and adds another check to avoid the bug described in #245. It's definitely a good approach for avoiding running during loadview so I made a commit to add this! I think the other performance improvement commits are still valid as well. Let me know what you think, but I think this PR is ready to go :)

Also just to clarify, this PR does resolve the problem that is described in #245 but does not directly resolve the bug in the load_oil_buffer function. If there are oil buffers not currently belonging to a window then it will clobber the current window. This still isn't fixed by this PR but it does a good job at avoiding that from happening.

Is there a chance when saving a session where an oil buffer could be saved while it doesn't belong to a window? If that's possible then this bug could still crop back up in the future if someone is an avid user of :mksession potentially.

mehalter commented 10 months ago

I removed my 2nd commit which checks if the filetype is oil or not. This is no less safe than checking if the buffer only has a single line. I'm not sure if that check should even be there in the first place. There could be many other reasons that a buffer has a single line that doesn't mean the buffer needs to have oil loaded, but that could be looked into separately.

For now it's best to just keep this PR minimal and implement the fix and performance improvements that make the most sense and are semantically sound.

For now this PR only does two things:

  1. Makes sure that the SessionLoadPost auto command only executes if a full session is loaded and not when :loadview is executed
  2. Only applies the SessionLoadPost auto command on the buffer that the event is called on. When sessions and views are loaded this auto command is executed on each and every buffer, so it is a huge performance hit to loop over each buffer. This performance gain moves execution complexity of the auto command from O(n²) to O(n) where n is the number of buffers getting restored

What this does not do:

stevearc commented 10 months ago

Thanks!