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

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

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

mehalter
Copy link
Contributor

@mehalter mehalter commented Dec 4, 2023

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: #246 (comment)

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.

Copy link
Owner

@stevearc stevearc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was originally intended for handling session files created with :mksession. At the time I didn't know about :mkview and :loadview, and didn't know that they also fired the same autocmd. What do you think of instead adding an early return at the top of this?

if vim.g.SessionLoad ~= 1 then
  return 
end

Then it should only run when sourcing a session file, not when doing :loadview (at least, in my testing the scripts created with :mkview don't set this variable). Does that also fix the issue for you?

@mehalter mehalter force-pushed the sessionloadpost_perf branch 2 times, most recently from 5bd5eb2 to 5bf8d1c Compare December 5, 2023 10:23
@mehalter
Copy link
Contributor Author

mehalter commented Dec 5, 2023

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 mehalter force-pushed the sessionloadpost_perf branch from 5bf8d1c to 6129a3f Compare December 5, 2023 18:21
@mehalter mehalter force-pushed the sessionloadpost_perf branch from 6129a3f to dd24c31 Compare December 5, 2023 18:23
@mehalter
Copy link
Contributor Author

mehalter commented Dec 5, 2023

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:

  • Fix the open issue in the load_oil_buffer function that can go and clobber active buffers if it gets applied to an Oil buffer that is not currently being served to a window. For example, if load_oil_buffer is called on an oil buffer number that is not currently belonging to a window, the buffer that is displayed in the current window will get it's filetype and syntax set to oil.
  • Improve the detection of buffers that actually need oil to be restored into them. Currently it checks if the scheme exists (good) and if the buffer only has a single line (arbitrary). Buffers can have a single line for a lot of different reasons: a file with a name that matches the oil schema with only a single line of content, an actual Oil buffer already loaded to a folder that only has a single file, etc.

@mehalter mehalter requested a review from stevearc December 5, 2023 18:47
@stevearc
Copy link
Owner

stevearc commented Dec 7, 2023

Thanks!

@stevearc stevearc merged commit b3c24f4 into stevearc:master Dec 7, 2023
7 checks passed
@mehalter mehalter deleted the sessionloadpost_perf branch December 7, 2023 10:25
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.

bug: Sometimes opening files when using mkview and loadview get messed up
3 participants