Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Get] Only instantiate Repo objects once for items in Packages. #276

Merged
merged 1 commit into from May 5, 2016

Conversation

ddunbar
Copy link
Member

@ddunbar ddunbar commented Apr 22, 2016

  • The old algorithm was O(N^2)... this gives a 3x speedup in null build time
    for a large project (Zewo).

 - The old algorithm was O(N^2)... this gives a 3x speedup in null build time
   for a large project (Zewo).
@ddunbar
Copy link
Member Author

ddunbar commented Apr 22, 2016

@mxcl can you review at some point? I'm not entirely sure this is safe, but it is a big win if so.

@mxcl
Copy link
Contributor

mxcl commented Apr 29, 2016

Possibly a problem because repo paths change during the Get and Update processes based on which checkouts are selected and the Git.Repo path is not changed.

Probably it's fine because the renames happen last. But may be better to just figure out why Repo.init is slow.

@ddunbar
Copy link
Member Author

ddunbar commented Apr 29, 2016

It is slow because it shells out to git to check things... that said I'm worried about anything which is O(N^2) even if we made it much cheaper...

I was worried about the situations you mentioned, but couldn't get it to fail in any cases. We could always force invalidation like the last bit of the patch does in any other places that updates the repo.

What should we do here? I'd really like to see something like this merged just for the Zewo perf win.

@mxcl
Copy link
Contributor

mxcl commented Apr 29, 2016

The initializer is:

public init?(path: String) {
    guard let realroot = try? realpath(path) else { return nil }
    self.path = realroot
    guard Path.join(path, ".git").isDirectory else { return nil }
}

It seems like origin is the slower function?

However I imagine this is safe since you identified the place where invalidation should happen.

I worry about this sort of thing however. The slow part of all this is the HTTP.

}

extension PackagesDirectory: Fetcher {
typealias T = Package

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove whitespace :)

@ddunbar
Copy link
Member Author

ddunbar commented May 5, 2016

I'm going to go ahead and take this once it passes CI, if there is fallout then that is probably useful to identify and codify in test cases, and I really want to see the perf win land.

@ddunbar
Copy link
Member Author

ddunbar commented May 5, 2016

@swift-ci please test and merge

@ddunbar
Copy link
Member Author

ddunbar commented May 5, 2016

Linux failure is unrelated.

@ddunbar ddunbar merged commit 38966d3 into apple:master May 5, 2016
@ddunbar ddunbar deleted the cache-Packages-Repo-objects branch May 17, 2016 06:47
ankitspd pushed a commit to ankitspd/swift-package-manager that referenced this pull request Jan 11, 2019
This adds new delegate methods for "missing inputs" and "multiple producers for a single node" errors. These provide context to the client now instead of constructing strings.

See <rdar://problem/34333034>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants