wting / gitsessions.vim

Auto save/load vim sessions based on directory and git branch.
MIT License
74 stars 17 forks source link

cache location of session file #10

Closed rleon closed 9 years ago

rleon commented 9 years ago

Function session_file() returns the name of session file. This function is called for every change in VIM layout.

Caching of return variable of this function gives x100 performance improvement.

Before change: FUNCTIONS SORTED ON TOTAL TIME count total (s) self (s) function 6 3.369383 0.015592 g:GitSessionUpdate() 6 3.353791 0.000861 35_session_file() 6 3.268404 0.001057 35_session_dir() 12 3.263580 0.004735 35_in_git_repo()

After this change: FUNCTIONS SORTED ON TOTAL TIME count total (s) self (s) function 11 0.026195 g:GitSessionUpdate() 23 0.025646 38_Highlight_Matching_Pair() 11 0.000956 nerdtree#checkForBrowse()

VIM profiling (http://stackoverflow.com/a/12216578): :profile start profile.log :profile func :profile file " At this point do slow actions :profile pause :noautocmd qall!

Signed-off-by: Leon Romanovsky leon@leon.nu

wting commented 9 years ago

Yeah, we're hitting the disk multiple times on when switching buffers. From your profiling it looks like +550 ms per buffer change, which is kind of excessive (maybe it's time to move to neovim for async). Was this tested on a spinning disk / network mount? Thanks for doing the leg work!

One concern is if a user switches branches but keeps vim open, they'll unknowingly continue saving to the previous session. I'm not sure if this is desired behavior or not. Currently the user doesn't have to think about invalidating caches when switching git branches.

If you still think this is worth pursuing, I have a few suggestions to make:

rleon commented 9 years ago

Thank you for taking time to review and provide such extensive feedback.

My testing environment is local SSD which is almost full (no space for over-provisioning) and the reason to such a large delays is related to the size of git repository (linux kernel).

I suggest to rewrite the session_file() function to return cached version if global variable is set. In case this variable is not set, the default behaviour will be performed.

Regarding the session change per branch, it was the easiest approach to fix performance issues and it fits perfectly my flow. I am open for suggestions how to get information on branch change in order to update session file location.

wting commented 9 years ago

Rewriting session_file() to use a cache is probably an easier to understand solution, and basically always invalidate the cache in GitSessionSave.

The more I think about it, the more I prefer this behavior. I've accidentally overwritten the wrong session because I leave vim open. I just want to put this behind a feature toggle so we can test this for a bit before making it the default behavior if everything looks good.

rleon commented 9 years ago

I agree with you, it is more convenient to user to save session once and use it for all branches.

The main concern is how to handle files which are available and open in one branch and not available in other.

In any cases, i'll update the pull request in day or two.

Thanks On Jul 23, 2015 9:33 PM, "William Ting" notifications@github.com wrote:

Rewriting session_file() to use a cache is probably an easier to understand solution, and basically always invalidate the cache in GitSessionSave.

The more I think about it, the more I prefer this behavior. I've accidentally overwritten the wrong session because I leave vim open. I just want to put this behind a feature toggle so we can test this for a bit before making it the default behavior if everything looks good.

— Reply to this email directly or view it on GitHub https://github.com/wting/gitsessions.vim/pull/10#issuecomment-124201814.

rleon commented 9 years ago

I pushed new version of this feature. Please review it.

Thanks.

rleon commented 9 years ago

set cached to be disabled

wting commented 9 years ago

Thanks for the patch and cleaning it up!

Just a heads up, I've renamed the caching option name to gitsessions_use_cache.

wting commented 9 years ago

I've updated docs here. Lemme know if it sounds weird or can be improved.

rleon commented 9 years ago

Thank you. It looks fine for me. I forgot to overlap g:cache_session_file variable. Can you please apply this change on top of my commit. diff --git a/plugin/gitsessions.vim b/plugin/gitsessions.vim index aa8a483..0f7e109 100644 --- a/plugin/gitsessions.vim +++ b/plugin/gitsessions.vim @@ -36,7 +36,9 @@ endif " Cons: switch between git branches will be missed from GitSessionUpdate() " You are advised to save it manually by calling to GitSessionSave() " Default - cache disabled -let g:cache_session_file = 0 +if !exists('g:cache_session_file')

wting commented 9 years ago

Yup, already added that in 9e444bcf49.

rleon commented 9 years ago

Thanks

wting commented 9 years ago

FYI this is the new default behavior as of d4e8c6515e.

rleon commented 9 years ago

Thank you, I appreciate it.