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.

Sign up for GitHub

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

Improve Carthage dependency lookup performance #298

Merged
merged 3 commits into from
Apr 24, 2018
Merged

Improve Carthage dependency lookup performance #298

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d4445c6
Store targets as dictionary for faster lookups
keith Apr 19, 2018
c4ac000
Iteratively discover carthage dependencies
keith Apr 19, 2018
812af8b
Update targetsMap on targets didSet
keith Apr 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions Sources/ProjectSpec/Project.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ public struct Project {

public var basePath: Path
public var name: String
public var targets: [Target]
public var targets: [Target] {
didSet {
self.targetsMap = Dictionary(uniqueKeysWithValues: self.targets.map { ($0.name, $0) })
}
}
public var settings: Settings
public var settingGroups: [String: Settings]
public var configs: [Config]
Expand All @@ -18,6 +22,7 @@ public struct Project {
public var fileGroups: [String]
public var configFiles: [String: String]
public var include: [String] = []
private var targetsMap: [String: Target]
Copy link
Owner

Choose a reason for hiding this comment

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

This should be recreated on the targets didSet as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, looks like this is only used for tests, seems like we should remove that mutability.

Copy link
Owner

Choose a reason for hiding this comment

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

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


public init(
basePath: Path,
Expand All @@ -35,6 +40,7 @@ public struct Project {
self.basePath = basePath
self.name = name
self.targets = targets
self.targetsMap = Dictionary(uniqueKeysWithValues: self.targets.map { ($0.name, $0) })
self.configs = configs
self.settings = settings
self.settingGroups = settingGroups
Expand All @@ -46,7 +52,7 @@ public struct Project {
}

public func getTarget(_ targetName: String) -> Target? {
return targets.first { $0.name == targetName }
return targetsMap[targetName]
}

public func getConfig(_ configName: String) -> Config? {
Expand Down Expand Up @@ -117,6 +123,7 @@ extension Project {
} else {
options = SpecOptions()
}
self.targetsMap = Dictionary(uniqueKeysWithValues: self.targets.map { ($0.name, $0) })
}

static func filterJSON(jsonDictionary: JSONDictionary) throws -> JSONDictionary {
Expand Down
39 changes: 22 additions & 17 deletions Sources/XcodeGenKit/PBXProjGenerator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -681,29 +681,34 @@ public class PBXProjGenerator {
return "\(carthagePath)/\(platformName)"
}

func getAllCarthageDependencies(target: Target, visitedTargets: [String: Bool] = [:]) -> [Dependency] {

func getAllCarthageDependencies(target: Target) -> [Dependency] {
// this is used to resolve cyclical target dependencies
var visitedTargets = visitedTargets
visitedTargets[target.name] = true

var visitedTargets: Set<String> = []
var frameworks: [Dependency] = []

for dependency in target.dependencies {
switch dependency.type {
case .carthage:
frameworks.append(dependency)
case .target:
let targetName = dependency.reference
if visitedTargets[targetName] == true {
return []
}
if let target = project.getTarget(targetName) {
frameworks += getAllCarthageDependencies(target: target, visitedTargets: visitedTargets)
var queue: [Target] = [target]
while !queue.isEmpty {
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is traversing the same dependencies yes. It should have the exact same result.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@yonaskolb The recursive version is passing visitedTargets by value, which I suspect is why the the iterative version is much faster.

let target = queue.removeFirst()
if visitedTargets.contains(target.name) {
continue
}

for dependency in target.dependencies {
switch dependency.type {
case .carthage:
frameworks.append(dependency)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this result in frameworks having duplicate dependencies in this list, and if so should this function return a unique list of dependencies?

Copy link
Owner

Choose a reason for hiding this comment

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

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

case .target:
if let target = project.getTarget(dependency.reference) {
queue.append(target)
}
default:
break
}
default: break
}

visitedTargets.update(with: target.name)
}

return frameworks
}
}
Expand Down