Repository navigation
perf(#3384): skip fnamemodify in is_git when GIT_DIR unset - #3388
Conversation
alex-courtis
left a comment
There was a problem hiding this comment.
This is great, thank you for your contribution.
Manual testing with log profile modified to track us:
Arch linux on ext4. linux source repo with max_folder_discovery = 5000
~8ms reduction for E
master
: ; grep "END.*is_git" $XDG_STATE_HOME/nvim/nvim-tree.log | sed -E 's/.*([0-9]+)us/\1/g' | paste -sd+ | bc
21555
: ; grep "END.*is_git" $XDG_STATE_HOME/nvim/nvim-tree.log | sed -E 's/.*([0-9]+)us/\1/g' | paste -sd+ | bc
21562
: ; grep "END.*is_git" $XDG_STATE_HOME/nvim/nvim-tree.log | sed -E 's/.*([0-9]+)us/\1/g' | paste -sd+ | bc
21376
branch
: ; grep "END.*is_git" $XDG_STATE_HOME/nvim/nvim-tree.log | sed -E 's/.*([0-9]+)us/\1/g' | paste -sd+ | bc
13027
: ; grep "END.*is_git" $XDG_STATE_HOME/nvim/nvim-tree.log | sed -E 's/.*([0-9]+)us/\1/g' | paste -sd+ | bc
12781
: ; grep "END.*is_git" $XDG_STATE_HOME/nvim/nvim-tree.log | sed -E 's/.*([0-9]+)us/\1/g' | paste -sd+ | bc
12947
| if vim.env.GIT_DIR and vim.fn.fnamemodify(path, ":p") == vim.fn.fnamemodify(vim.env.GIT_DIR, ":p") then | ||
| return true | ||
| elseif vim.fn.fnamemodify(path, ":t") == ".git" then | ||
| elseif utils.path_basename(path) == ".git" then -- pure lua equivalent of fnamemodify :t |
There was a problem hiding this comment.
Please refrain from comments like these: it's only relevant to explain this change and it doesn't add any value for future maintainers.
There was a problem hiding this comment.
true, this was more of self-reminder i forgot to amend, mb. removed now
That's sensible. We've had several issues in the past where we were too aggressive on git caching. |
In the future please provide instructions for performance testing an replication, to lessen the burden of review. |
|
Thank you a lot for reviewing, @alex-courtis ❤️ ! Though, and please correct me if i'm wrong, I think I spotted an issue with your measurements: I think the sed is under-reporting. That greedy Repro of the issue: |
Good catch! With the correct regex: master: branch: |
|
I'm really happy with this PR and the issues you have raised, which will enable great performance increases for the tree. Your descriptions are well written and readable. PRs for all those issues would be most gratefully appreciated. Please remember:
|
In
explorer/watch.lua,is_gitexecutes for every directory node assigned a filesystem watcher.Previously, even when
GIT_DIRwas unset, the function invoked expensive Vimscript evaluations, callingvim.fn.fnamemodify(path, ":p"),fnamemodify(nil, ":p"), andfnamemodify(path, ":t")on every check 🫠 .This change basically guards the
GIT_DIRresolution behind an environment check and replacesfnamemodify(..., ":t")with path extraction in purelua, eliminating the Vimscript round-trips during directory scanning.I thought we could even cache
GIT_DIR, but that would expose us to the risk of not catching up if the env var gets changed at runtime, uncommon but better safe than sorry 🤷 .Note I used
utils.path_basename(path)as the lua equivalent offnamemodify(path, ":t”), as I already noticed this was its use in the pre-existing codebase.Measured impact:
is_gitacross ~1,5k directories dropped from ~130 ms down to ~2.5 ms (~50x speedup) (!!)AI usage: Claude Code (Opus 5.5) helped on self-review, code cleanup and most importantly measuring the impact.
Tested on macOS nvim-dev 0.13, untested on Windows (though
path_basenamerelies onpackage.configpath separators), Linux, Neovim 0.10/0.11, and scenarios whereGIT_DIRis explicitly defined.resolves #3384