Faster multiple file staging #629

lapfelix · 2019-12-24T20:45:27Z

Staging/unstaging/discarding multiple files takes a long time. Seems to be O(N) where N is multiplied by 1-2 seconds, depending on the repo.

I fixed it by only calling -[GCRepository writeRepositoryIndex:error:] once when doing staging/discarding operations on multiple files by adding methods for these operations that take multiple files instead of one.

I tested the changes in a big repository (with thousands of commits, ~50 branches and ~20-30 total submodules) and the time it takes to stage/unstage 19 files went from ~25.5 seconds to ~2.5 seconds. The more files are modified, the bigger the performance increase is.

I AGREE TO THE GITUP CONTRIBUTOR LICENSE AGREEMENT

lucasderraugh · 2019-12-25T02:21:12Z

Absolutely wonderful! I’m out for Christmas but I will gladly review this when I’m back on the 27th. 🍻

lucasderraugh

Looks pretty good to me. A few points to address but overall I'm excited about this change 😃.

GitUpKit/Extensions/GCRepository+Index.m

GitUpKit/Utilities/GIViewController+Utilities.h

GitUpKit/Views/GIAdvancedCommitViewController.m

lucasderraugh

Oops, meant to request changes.

lucasderraugh

Looks good! I'll give it a spin for performance soon and then merge it in.

lucasderraugh

Sorry, just one last thing to fix. I think we need to be freeing that malloced region before this function returns and it doesn't have to be + 1 count worth of space.

GitUpKit/Core/GCIndex.m

The selectedFiles array would always be empty because the `else` should've been outside the `if (submodule)`

lapfelix · 2020-01-07T05:21:58Z

I've pushed the requested change and fixed a bug in the code that calls checkoutFilesToWorkingDirectory (where it never sent the multiple selected file delta paths) and added comments to make it easier to understand for the next person reading it.

lucasderraugh · 2020-01-12T18:10:30Z

The speed boost is fantastic. Thanks for this!

lucasderraugh · 2020-01-23T00:06:59Z

@lapfelix I ended up reverting this change unfortunately. There are some major issues with discarding selected files that need resolution.

lapfelix · 2020-01-23T00:07:39Z

Oh no! Do you have any steps to reproduce the issue?

lapfelix · 2020-01-23T00:08:06Z

Okay I see the linked issue

lucasderraugh · 2020-01-23T00:09:25Z

Ya, my repro steps were just selecting a few files and hitting the delete key to discard. I had cases where it would not notify about the change, but I also noticed that sometimes it would discard things I didn't select, so it had to be reverted for that reason.

lucasderraugh · 2020-01-23T00:11:49Z

GitUpKit/Views/GIAdvancedCommitViewController.m

+
+                                       NSError* error;
+                                       if (![self discardAllChangesForFiles:selectedFiles resetIndex:NO error:&error]) {
+                                         [self.repository notifyWorkingDirectoryChanged];


I believe this was the issue around why it didn't notify when discarding files. This code would only notify on failures basically. The other issue where it would discard things I didn't select was the most concerning however and I'm not sure what the cause of that was.

Ugh yeah the notifyWorkingDirectoryChanged and makeFirstResponder lines definitely shouldn't be in that if.

I'll review the rest of the code and see if I can reproduce that otgher issue where more things were discarded than necessary.

I really can't reproduce the other issue (with these lines placed at the right place). I wonder if it might be related to your other issue where the wrong files were discarded though. I'll revert my fix and see if I can make it happen like this.

Okaaaaayyyy I've reproduced it. It's bad.

Looks like the issue is in the call to the libgit2 function git_checkout_index. Right before it's called, the repo is fine (the right files have been deleted) but it seems like it resets too many files.

lucasderraugh · 2020-01-23T02:40:47Z

Ya, perhaps it’s a bug on their end? We’ll have to update libgit at some point.

lapfelix · 2020-01-23T02:57:36Z

@lucasderraugh Hmmmm I have a fix which is to remove the GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH option in checkoutFilesToWorkingDirectory: but I'll have to figure out why it was there in the first place

lapfelix · 2020-01-23T03:52:27Z

@lucasderraugh I've opened a PR with the changes I'm testing: #639

lucasderraugh · 2020-01-23T03:58:14Z

Definitely want to know what that flag does. Thanks for looking into it though!

lapfelix · 2020-01-23T15:42:01Z

One thing we could do to work around this apparent (?) bug is remove the flag and escape the paths we pass in so file names aren't recognized as match patterns. I've tried it and it works. I'll try it with filenames that might get recognized as match patterns before committing this change.

lucasderraugh · 2020-01-23T19:58:02Z

I'll have to look up what this match pattern stuff means 😊. One other concern I have that I'd like to test out is what happens on failure? Previously just 1 file was skipped over, but in this case does every file fail to get merged? I'm thinking of the discard case where perhaps you have submodules and you try to discard them and a few other files but one thing fails so the entire discard fails.

lapfelix · 2020-01-25T19:39:08Z

@lucasderraugh Submodules are still handled individually so if there's an error discarding a submodule, it will happen before we batch-process the files so we keep the same behaviour in this specific file. It will abort once it hits an error discarding a submodule.

When discarding changes in multiple files (deleting them then doing a checkout), if there's an error when deleting the file (not sure how often this happens, but it could), it will stop deleting files and will abort, present an error and stay where it was (which means the files that were already successfully deleted will stay deleted). So in that case, we won't do the checkout for these already deleted files.

If it successfully deletes the files and then fails when doing the batch-checkout, I'm not sure what will happen. I guess it will fail for all of them, yes. I haven't encountered that yet.

3f5abaf

683f6de

* Improve performance when staging/unstaging multiple files at once * Optimize multiple files discarding * Run ClangFormat * Changes following PR feedback * Fix bug caused by bad merge and add comments The selectedFiles array would always be empty because the `else` should've been outside the `if (submodule)` * Correctly allocate and free multiple path strings

3f5abaf

* Revert "Revert "Faster multiple file staging (#629)"" 683f6de * Fix issue where repository wasn't notified of changes * Remove `GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH` as checkout strategy option * Escape paths to prevent them from being recognized as match patterns

lapfelix and others added 3 commits December 21, 2019 17:39

Improve performance when staging/unstaging multiple files at once

45659f5

Optimize multiple files discarding

8ae8287

Run ClangFormat

ec444ac

lucasderraugh approved these changes Dec 28, 2019

View reviewed changes

lucasderraugh requested changes Dec 28, 2019

View reviewed changes

Changes following PR feedback

80177eb

lapfelix requested a review from lucasderraugh January 2, 2020 17:17

lucasderraugh approved these changes Jan 2, 2020

View reviewed changes

lucasderraugh requested changes Jan 5, 2020

View reviewed changes

GitUpKit/Core/GCIndex.m Outdated Show resolved Hide resolved

lapfelix added 2 commits January 7, 2020 00:16

Fix bug caused by bad merge and add comments

e7c3b3a

The selectedFiles array would always be empty because the `else` should've been outside the `if (submodule)`

Correctly allocate and free multiple path strings

38ee8b7

lapfelix requested a review from lucasderraugh January 7, 2020 05:18

lucasderraugh approved these changes Jan 12, 2020

View reviewed changes

lucasderraugh merged commit 3f5abaf into git-up:master Jan 12, 2020

lucasderraugh mentioned this pull request Jan 22, 2020

Diff view not updating on files changes being discarded #638

Closed

lucasderraugh reviewed Jan 23, 2020

View reviewed changes

lucasderraugh added a commit that referenced this pull request Jan 27, 2020

Revert "Faster multiple file staging (#629)"

683f6de

3f5abaf

lapfelix added a commit to lapfelix/GitUp that referenced this pull request Jan 27, 2020

Revert "Revert "Faster multiple file staging (git-up#629)""

58ff20d

683f6de

lapfelix mentioned this pull request Jan 27, 2020

Fix issues caused by batch file staging/discard changes #639

Merged

simpzan pushed a commit to simpzan/GitUp that referenced this pull request Oct 22, 2020

Revert "Faster multiple file staging (git-up#629)"

58c9a39

3f5abaf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster multiple file staging #629

Faster multiple file staging #629

lapfelix commented Dec 24, 2019

lucasderraugh commented Dec 25, 2019

lucasderraugh left a comment

lucasderraugh left a comment

lucasderraugh left a comment

lucasderraugh left a comment

lapfelix commented Jan 7, 2020

lucasderraugh commented Jan 12, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lucasderraugh Jan 23, 2020 •

edited

Loading

lapfelix Jan 23, 2020

lapfelix Jan 23, 2020

lapfelix Jan 23, 2020

lapfelix Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 25, 2020

Faster multiple file staging #629

Faster multiple file staging #629

Conversation

lapfelix commented Dec 24, 2019

lucasderraugh commented Dec 25, 2019

lucasderraugh left a comment

Choose a reason for hiding this comment

lucasderraugh left a comment

Choose a reason for hiding this comment

lucasderraugh left a comment

Choose a reason for hiding this comment

lucasderraugh left a comment

Choose a reason for hiding this comment

lapfelix commented Jan 7, 2020

lucasderraugh commented Jan 12, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lucasderraugh Jan 23, 2020 • edited Loading

Choose a reason for hiding this comment

lapfelix Jan 23, 2020

Choose a reason for hiding this comment

lapfelix Jan 23, 2020

Choose a reason for hiding this comment

lapfelix Jan 23, 2020

Choose a reason for hiding this comment

lapfelix Jan 23, 2020

Choose a reason for hiding this comment

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 23, 2020

lucasderraugh commented Jan 23, 2020

lapfelix commented Jan 25, 2020

lucasderraugh Jan 23, 2020 •

edited

Loading