Improve submodule operation performance #2655

CosynPa · 2018-12-02T11:55:35Z

Fixes #1833 and #2651.

Better wait after #2654 is merged, then rebase on it.

tmspzz · 2018-12-04T22:21:37Z

You can rebase

…ation for the recursive submodule cloning

…sitory are left for later commits

CosynPa · 2018-12-07T10:45:53Z

Rebased.

tmspzz · 2018-12-07T11:34:09Z

I don't work with submodules enough to feel qualified to review the change. I think @mdiep should be reviewing here.

@CosynPa Ad high level description of the problem and the solution would certainly sleep up the review.

CosynPa · 2018-12-07T13:42:56Z

The problem is, as is pointed out in the two issues, the carthage checkout operation will try cloning submodules of dependencies, which takes a lot of time.

There are two functions that will clone or fetch submodules, one is cloneRepository which will be called when the --use-submodules option is not used, the other is checkoutSubmodule which will be called both with or without the --use-submodules option.

My solution is that first clone or fetch submodules to the local cache directory. Then redirect the slow network operations to the local cache. For checkoutSubmodule, which will do a git submodule update --init --recursive, the solution is a little bit tricky. I first change the .gitmodules file, so that the URLs of the submodules are pointing to the local cache repository, then do the git submodule update --init, and change the URLs back.

I also serialized heavy repository write operations, so cloning the same repository twice and checking out the same repository twice can be avoided.

mdiep · 2018-12-23T00:12:33Z

I'll try to get to this ASAP. I'm on vacation right now, but I'll see if I can find some time to review this.

stale · 2019-01-22T19:31:48Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

mdiep · 2019-01-24T13:27:38Z

Source/CarthageKit/Git.swift

@@ -153,6 +153,7 @@ public func contentsOfFileInRepository(_ repositoryFileURL: URL, _ path: String,
 public func checkoutRepositoryToDirectory(
 	_ repositoryFileURL: URL,
 	_ workingDirectoryURL: URL,
+	force: Bool,


Why was this added/needed?

The original checkoutRepositoryToDirectory function was a forced checkout, and in checkoutSubmodule there was a non-forced checkout. So to reuse the checkoutRepositoryToDirectory function, I added the force parameter.

mdiep · 2019-01-24T13:32:38Z

Source/CarthageKit/Project.swift

+
+	/// Clones the given submodule into the working directory of its parent
+	/// repository, but without any Git metadata.
+	public func cloneSubmoduleInWorkingDirectory(_ submodule: Submodule, _ workingDirectoryURL: URL) -> SignalProducer<(), CarthageError> {


I don't think these functions should move here:

These are generic functions that shouldn't really be tied to the Project

Because these were moved, the diff for this PR is really large and almost impossible to review

Can you move them back to the Git file?

These functions were moved because the new implementation depends on repositoryFileURL which is a private function in Project.swift.

Then that should be passed in as a parameter

What about cloneOrFetchDependency? These functions actually rely on cloneOrFetchDependency which uses cloneOrFetchQueue and _projectEventsObserver which are Project properties. I think some kind of queue is definitely needed. So also pass the queue in as a parameter?

mdiep · 2019-01-24T13:36:40Z

Source/CarthageKit/Project.swift

+			}
+		}
+	}
+}


This global state makes me very uncomfortable. I don't think it's a good idea to introduce a static var with this. It leads to code that's hard to follow and modify.

We should use another method to enforce exclusivity. I think it makes sense to do something like this:

Fetch all the submodules from the repository cache, in parallel but with limited concurrency.

Perform the actual checkouts, passing in a queue or doing them one at a time

I see there are already a lot of queues, but they are instance properties of a Project. So I just need to move the global variable to the Project class?

mdiep · 2019-01-24T13:45:04Z

Source/CarthageKit/Project.swift

+	}
+
+	public func updateSubmodules(_ superprojectDirectoryURL: URL) -> SignalProducer<(), CarthageError> {
+		func changeGitmodulesUrls(toCacheURL: Bool) -> SignalProducer<(), CarthageError> {


I don't think this is necessary. The original code in addSubmoduleToRepository already handled doing the equivalent of git submodule update --init from the repository cache.

If your intent is to make that logic recursive, I think you can do that following the existing method without modifying .gitmodules.

mdiep · 2019-01-24T13:48:06Z

First off: Thanks for the PR!

I think the overall idea—make sure we're using the repository cache for the submodules of dependencies—is very good. That would be a very nice contribution.

But I'm not comfortable with the current approach. I think it needs to be reworked to (1) be more reviewable and (2) avoid changing .gitmodules.

I also serialized heavy repository write operations, so cloning the same repository twice and checking out the same repository twice can be avoided.

Is this necessary for the primary change described above? If not, let's leave it for a followup PR. Bundling changes together makes PRs harder, and thus slower, to review—which is worse experience for everyone.

CosynPa · 2019-05-26T13:45:27Z

I'd like to separate this PR into two. The first is #2795.

CosynPa · 2019-08-04T08:38:39Z

I've separated this PR into three: #2795, #2832 and #2842.

1ec5 · 2020-01-09T18:44:08Z

Looks like both #2795 and #2832 have been merged, but #2842 got closed as stale.

CosynPa force-pushed the submodule-performance branch from 5e9c436 to 824648d Compare December 3, 2018 04:07

CosynPa added 8 commits December 7, 2018 18:01

Move a function

043bd12

Clone submodules from the cache.

a8eb861

Add a force parameter

05660ea

Avoid direct git recursive submodule update, use the cached implement…

28e76ce

…ation for the recursive submodule cloning

Move addSubmoduleToRepository to Project

00394b1

Serialize repository operations. The operations in addSubmoduleToRepo…

b748b5b

…sitory are left for later commits

Optimize addSubmoduleToRepository.

977bf9c

Remove an unused function

abd72ca

CosynPa force-pushed the submodule-performance branch from 824648d to abd72ca Compare December 7, 2018 10:21

stale bot added the stale label Jan 22, 2019

tmspzz added the enhancement label Jan 22, 2019

stale bot removed the stale label Jan 22, 2019

mdiep requested changes Jan 24, 2019

View reviewed changes

1ec5 mentioned this pull request Jan 9, 2020

Checkout spends a lot of time cloning submodules #1833

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve submodule operation performance #2655

Improve submodule operation performance #2655

CosynPa commented Dec 2, 2018

tmspzz commented Dec 4, 2018

CosynPa commented Dec 7, 2018

tmspzz commented Dec 7, 2018

CosynPa commented Dec 7, 2018 •

edited

mdiep commented Dec 23, 2018

stale bot commented Jan 22, 2019

mdiep Jan 24, 2019

CosynPa Mar 27, 2019

mdiep Jan 24, 2019

CosynPa Mar 27, 2019

mdiep Mar 28, 2019

CosynPa Mar 30, 2019

mdiep Jan 24, 2019

CosynPa Mar 27, 2019

mdiep Jan 24, 2019

mdiep commented Jan 24, 2019

CosynPa commented May 26, 2019

CosynPa commented Aug 4, 2019

1ec5 commented Jan 9, 2020

Improve submodule operation performance #2655

Are you sure you want to change the base?

Improve submodule operation performance #2655

Conversation

CosynPa commented Dec 2, 2018

tmspzz commented Dec 4, 2018

CosynPa commented Dec 7, 2018

tmspzz commented Dec 7, 2018

CosynPa commented Dec 7, 2018 • edited

mdiep commented Dec 23, 2018

stale bot commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdiep commented Jan 24, 2019

CosynPa commented May 26, 2019

CosynPa commented Aug 4, 2019

1ec5 commented Jan 9, 2020

CosynPa commented Dec 7, 2018 •

edited