Improve Carthage dependency lookup performance #298
Conversation
|
This produces the same project for us and passes the tests, but since we don't have any carthage dependencies it would be useful if someone else could verify it still produces a valid project for them. |
There was a problem hiding this comment.
I tested with my open source Canvas project. I ran the following commands to verify the project is identical before and after.
$ git clone https://github.com/soffes/Canvas && cd Canvas
$ git clone https://github.com/yonaskolb/XcodeGen
$ cd XcodeGen && swift build && cd ..
$ ./XcodeGen/.build/debug/xcodegen
$ cp Canvas.xcodeproj/project.pbxproj project.pbxproj.orig
$ rm -rf XcodeGen/.build
$ cd XcodeGen && git checkout ks/carthage-performance && swift build && cd ..
$ ./XcodeGen/.build/debug/xcodegen
$ cmp Canvas.xcodeproj/project.pbxproj project.pbxproj.orig && echo "Same!"After running carthage bootstrap it builds fine before and after.
| for dependency in target.dependencies { | ||
| switch dependency.type { | ||
| case .carthage: | ||
| frameworks.append(dependency) |
There was a problem hiding this comment.
Can this result in frameworks having duplicate dependencies in this list, and if so should this function return a unique list of dependencies?
There was a problem hiding this comment.
The carthage dependencies are de-duplicated later on when they are added to a target. This could be done here, but maybe it might be useful at some point to know how many times a dependency is being used
| @@ -18,6 +18,7 @@ public struct Project { | |||
| public var fileGroups: [String] | |||
| public var configFiles: [String: String] | |||
| public var include: [String] = [] | |||
| private var targetsMap: [String: Target] | |||
There was a problem hiding this comment.
This should be recreated on the targets didSet as well
There was a problem hiding this comment.
done, looks like this is only used for tests, seems like we should remove that mutability.
There was a problem hiding this comment.
The ProjectSpec module can also be used as a framework in other projects.
Personally I've never seen the point of making properties on a Struct immutable, as the property referencing that struct can just be a let to control the immutability from a single place, while still allowing easy mutability for other cases without reaching into a full blown initializer
| for dependency in target.dependencies { | ||
| switch dependency.type { | ||
| case .carthage: | ||
| frameworks.append(dependency) |
There was a problem hiding this comment.
The carthage dependencies are de-duplicated later on when they are added to a target. This could be done here, but maybe it might be useful at some point to know how many times a dependency is being used
| if let target = project.getTarget(targetName) { | ||
| frameworks += getAllCarthageDependencies(target: target, visitedTargets: visitedTargets) | ||
| var queue: [Target] = [target] | ||
| while !queue.isEmpty { |
There was a problem hiding this comment.
Is this traversing the same graph, but just doing it in one function instead of recursively?
I think to get this really performant the carthage dependencies for a target could be cached by just running this once for all targets at the beginning, instead of each target going through the whole graph again
There was a problem hiding this comment.
It is traversing the same dependencies yes. It should have the exact same result.
There was a problem hiding this comment.
We could do that, I figured that would be a larger enough change that if we could do this fast enough it's not really worth it.
There was a problem hiding this comment.
@yonaskolb The recursive version is passing visitedTargets by value, which I suspect is why the the iterative version is much faster.
|
Thanks! |
Previously for large projects, project generation time could be up to 40 seconds in our testing because of lookup up Carthage dependencies (regardless of if you have any). This change improves the performance of this with 2 changes.
getTarget