Repository navigation
Performance and UI responsiveness #212
Description
After trying out the package with the new shell-out strategy, I have noticed a significant regression in terms of performance when interacting with the UI. Enabling global.PRINT_GIT_TIMES shows that we are performing some operations more than once when refreshing Repository, thus decreasing UI responsiveness (i.e. a single refresh seems to take ~ 470ms). Here's the log for staging a file:
git add .gitignore (stdin length null)@1471801160523: 7.604ms
git diff --no-prefix --no-renames --staged (stdin length null)@1471801160540: 7.737ms
git diff --name-status --no-renames --staged (stdin length null)@1471801160551: 8.499ms
git clean --dry-run (stdin length null)@1471801160563: 11.264ms
git diff --name-status --no-renames --diff-filter=U (stdin length null)@1471801160585: 15.920ms
git diff --no-prefix --no-renames --staged (stdin length null)@1471801160602: 12.869ms
git clean --dry-run (stdin length null)@1471801160616: 10.037ms
git diff --name-status --no-renames --staged (stdin length null)@1471801160629: 7.910ms
git diff --no-prefix --no-renames (stdin length null)@1471801160638: 17.785ms
git clean --dry-run (stdin length null)@1471801160658: 12.219ms
git diff --name-status --no-renames (stdin length null)@1471801160671: 15.764ms
git diff --name-status --no-renames --diff-filter=U (stdin length null)@1471801160688: 19.322ms
git clean --dry-run (stdin length null)@1471801160710: 11.273ms
git clean --dry-run (stdin length null)@1471801160723: 10.170ms
git clean --dry-run (stdin length null)@1471801160734: 16.023ms
git diff --no-prefix --no-renames (stdin length null)@1471801160751: 16.642ms
git diff --name-status --no-renames --diff-filter=U (stdin length null)@1471801160769: 18.069ms
git diff --name-status --no-renames (stdin length null)@1471801160788: 16.039ms
git clean --dry-run (stdin length null)@1471801160806: 12.833ms
git clean --dry-run (stdin length null)@1471801160820: 38.309ms
git status --short (stdin length null)@1471801160861: 36.325ms
git clean --dry-run (stdin length null)@1471801160899: 24.589ms
git diff --name-status --no-renames HEAD (stdin length null)@1471801160926: 28.138ms
git diff --name-status --no-renames --diff-filter=U (stdin length null)@1471801160955: 16.008ms
git clean --dry-run (stdin length null)@1471801160972: 19.582ms
refresh: 470.006ms
git clean --dry-run (stdin length null)@1471801160994: 11.996ms
git status --short (stdin length null)@1471801161009: 25.620ms
git diff --name-status --no-renames HEAD (stdin length null)@1471801161036: 19.198ms
git clean --dry-run (stdin length null)@1471801161058: 18.091ms
git rev-parse --abbrev-ref HEAD (stdin length null)@1471801161085: 7.098ms
git rev-parse --abbrev-ref HEAD (stdin length null)@1471801161095: 7.389ms
git config branch.master.remote (stdin length null)@1471801161104: 8.466ms
git config branch.master.remote (stdin length null)@1471801161114: 54.136ms
git config branch.master.remote (stdin length null)@1471801161169: 6.943ms
git config branch.master.remote (stdin length null)@1471801161179: 9.376ms
git rev-list origin/master..master (stdin length null)@1471801161191: 12.594ms
git rev-list origin/master..master (stdin length null)@1471801161206: 13.113ms
git config branch.master.remote (stdin length null)@1471801161221: 8.768ms
git config branch.master.remote (stdin length null)@1471801161233: 8.461ms
git rev-list master..origin/master (stdin length null)@1471801161244: 9.559ms
git rev-list master..origin/master (stdin length null)@1471801161257: 33.827ms
This might be caused by some event being triggered more than once (e.g. two consecutive changes in the filesystem), which cause Repository to be refreshed multiple times before rendering its state in the UI. I haven't investigated if this hypothesis is valid, but if so it's probably worth performing refreshes in the Repository lazily now that I/O has a higher cost.
On a side note, it seems also like spawning git via child_process has a non-negligible cost: once we have removed all the aforementioned duplicate calls, I wonder if it might be worth keeping open that communication channel somehow, so that we can just send data back and forth in later calls.
/cc: @kuychaco
Activity
kuychaco commentedon Aug 23, 2016
Thanks for looking into this @as-cii! Those are some great thoughts, will explore performance improvements when I get back. I think we can also decrease the number of times we're shelling out by using different Git commands to get more information in one shot.
as-cii commentedon Sep 27, 2016
Based on our yesterday's conversation about performance, this morning I took some time to benchmark (un)staging for small files (~ 100-200 lines) and jotted down some notes:
Repository.prototype.refresh()could be parallelized withPromise.allRepository.prototype.refresh()gets called twice after any given operation: this might be happening because we call it manually in methods likestageFiles, and automatically as part of theFileSystemChangeObserver(because git files change after making modifications to the index)Repository.prototype.refresh()breakdown:git.diff({staged: true}): 15-20msgit.diff(): 57-60msgit.getMergeConflictFileStatus(): 80-100msRepository.prototype.getMergeConflictFileStatus()can be parallelized withPromise.allRepository.prototype.diffFileStatuscan be parallelized withPromise.allGitPanelController.prototype._refreshModelDatacan be parallelized.git diff --name-status --no-renames HEADtakes ~50ms, can we replace it withgit status -z? Seems 2x faster.git ls-files --others --exclude-standard(call count: 2)git config branch.branch-name.remote(call count: 3)git rev-list origin/as-deprecate-shadow-dom..as-deprecate-shadow-domandgit rev-list as-deprecate-shadow-dom..origin/as-deprecate-shadow-dom. Haven't looked at the code, but could we use againgit statushere and parse "ahead/behind count" output? Alternatively,git rev-list --left-right local…remotecould help us save a call.I haven't looked at the etch part yet, but I suspect (most of) the perceived long latency might be attributable to performing the above operations. Before exploring tiling (which we might still want to do, especially considering the big files scenarios mentioned yesterday), I feel like we should get a baseline with smaller files first.
/cc: @atom/core
annthurium commentedon Nov 30, 2018
I'm going to assume this should be closed out as it's old and we've done significant performance work in the past 2 years. Feel free to reopen if it's still an issue.