From e7f753785e6ee135e5ea77268d149e9cd10bb7ed Mon Sep 17 00:00:00 2001 From: Mathieu Olivari <1377279+ma-oli@users.noreply.github.com> Date: Thu, 3 Nov 2022 01:05:46 -0700 Subject: [PATCH] Fix includes related issues and improve their performances (#1275) * Fix recursive include path when relativePath is not set If relativePath is not set on a particular include, the first level of include will currently work, but starting at the second level of iteration, the computed include path will fail as relativePath will be appended over and over onto the filePath. We're fixing that recursion problem here and adding the corresponding tests to make sure it doesn't happen again. * Include projectRoot in include paths The projectRoot setting (when specified) is currently ignored when computing the include paths. We're fixing that in that commit. * Use memoization during recursive SpecFiles creation SpecFile objects are created by recursive through includes. On a large project with programatically generated SpecFile, it is not rare to have hundreds of SpecFiles, creating a large web of include dependencies. In such a case, it is not rare either for a particular SpecFile to be included by multiple other SpecFiles. When that happens, XcodeGen currently creates a SpecFile object every time a SpecFile gets included, which can lead to an exponential growth of includes. I have seen hundreds of files being turned into hundred of thousands of SpecFile object creations, which leads to an impractical XcodeGen run of tens of minutes. This change adds memoization during SpecFile recursion, in order to reuse the previously created SpecFiles, if available, instead of re-creating them. * Update CHANGELOG.md Add the following changes to the changelog: * b97bdc4 - Use memoization during recursive SpecFiles creation * a6b96ad - Include projectRoot in include paths * 557b074 - Fix recursive include path when relativePath is not set --- CHANGELOG.md | 3 ++ Sources/ProjectSpec/Project.swift | 9 ++++- Sources/ProjectSpec/SpecFile.swift | 39 ++++++++++++------- Sources/ProjectSpec/SpecLoader.swift | 3 +- .../legacy_included_paths_test.yml | 5 ++- .../legacy_paths_test/recursive_include.yml | 4 ++ Tests/PerformanceTests/PerformanceTests.swift | 5 ++- 7 files changed, 49 insertions(+), 19 deletions(-) create mode 100644 Tests/Fixtures/legacy_paths_test/recursive_include.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c741eaf0..b4af2736c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,13 @@ - Added support for `enableGPUFrameCaptureMode` #1251 @bsudekum - Config setting presets can now also be loaded from the main bundle when bundling XcodeGenKit #1135 @SofteqDG - Added ability to generate multiple projects in one XcodeGen launch #1270 @skofgar +- Use memoization during recursive SpecFiles creation. This provides a drastic performance boost with lots of recursive includes #1275 @ma-oli ### Fixed - Fix scheme not being generated for aggregate targets #1250 @CraigSiemens +- Fix recursive include path when relativePath is not set #1275 @ma-oli +- Include projectRoot in include paths #1275 @ma-oli ## 2.32.0 diff --git a/Sources/ProjectSpec/Project.swift b/Sources/ProjectSpec/Project.swift index bc82e9aa5..4b325eab9 100644 --- a/Sources/ProjectSpec/Project.swift +++ b/Sources/ProjectSpec/Project.swift @@ -155,7 +155,14 @@ extension Project: Equatable { extension Project { public init(path: Path) throws { - let spec = try SpecFile(path: path) + var cachedSpecFiles: [Path: SpecFile] = [:] + let spec = try SpecFile(filePath: path, basePath: path.parent(), cachedSpecFiles: &cachedSpecFiles) + try self.init(spec: spec) + } + + public init(path: Path, basePath: Path) throws { + var cachedSpecFiles: [Path: SpecFile] = [:] + let spec = try SpecFile(filePath: path, basePath: basePath, cachedSpecFiles: &cachedSpecFiles) try self.init(spec: spec) } diff --git a/Sources/ProjectSpec/SpecFile.swift b/Sources/ProjectSpec/SpecFile.swift index 6a1c59b43..7667a821c 100644 --- a/Sources/ProjectSpec/SpecFile.swift +++ b/Sources/ProjectSpec/SpecFile.swift @@ -48,8 +48,8 @@ public struct SpecFile { } } - public init(path: Path, variables: [String: String] = [:]) throws { - try self.init(filePath: path, basePath: path.parent(), variables: variables) + public init(path: Path, cachedSpecFiles: inout [Path: SpecFile], variables: [String: String] = [:]) throws { + try self.init(filePath: path, basePath: path.parent(), cachedSpecFiles: &cachedSpecFiles, variables: variables) } public init(filePath: Path, jsonDictionary: JSONDictionary, basePath: Path = "", relativePath: Path = "", subSpecs: [SpecFile] = []) { @@ -60,25 +60,29 @@ public struct SpecFile { self.filePath = filePath } - private init(include: Include, basePath: Path, relativePath: Path, variables: [String: String]) throws { - let basePath = include.relativePaths ? (basePath + relativePath) : (basePath + relativePath + include.path.parent()) + private init(include: Include, basePath: Path, relativePath: Path, cachedSpecFiles: inout [Path: SpecFile], variables: [String: String]) throws { + let basePath = include.relativePaths ? (basePath + relativePath) : basePath let relativePath = include.relativePaths ? include.path.parent() : Path() + let includePath = include.relativePaths ? basePath + relativePath + include.path.lastComponent : basePath + include.path - try self.init(filePath: include.path, basePath: basePath, variables: variables, relativePath: relativePath) + try self.init(filePath: includePath, basePath: basePath, cachedSpecFiles: &cachedSpecFiles, variables: variables, relativePath: relativePath) } - private init(filePath: Path, basePath: Path, variables: [String: String], relativePath: Path = "") throws { - let path = basePath + relativePath + filePath.lastComponent - let jsonDictionary = try SpecFile.loadDictionary(path: path).expand(variables: variables) - + public init(filePath: Path, basePath: Path, cachedSpecFiles: inout [Path: SpecFile], variables: [String: String] = [:], relativePath: Path = "") throws { + let jsonDictionary = try SpecFile.loadDictionary(path: filePath).expand(variables: variables) let includes = Include.parse(json: jsonDictionary["include"]) let subSpecs: [SpecFile] = try includes .filter(\.enable) .map { include in - try SpecFile(include: include, basePath: basePath, relativePath: relativePath, variables: variables) + if let specFile = cachedSpecFiles[filePath] { + return specFile + } else { + return try SpecFile(include: include, basePath: basePath, relativePath: relativePath, cachedSpecFiles: &cachedSpecFiles, variables: variables) + } } self.init(filePath: filePath, jsonDictionary: jsonDictionary, basePath: basePath, relativePath: relativePath, subSpecs: subSpecs) + cachedSpecFiles[filePath] = self } static func loadDictionary(path: Path) throws -> JSONDictionary { @@ -100,7 +104,8 @@ public struct SpecFile { } private func resolvedDictionaryWithUniqueTargets() -> JSONDictionary { - let resolvedSpec = resolvingPaths() + var cachedSpecFiles: [Path: SpecFile] = [:] + let resolvedSpec = resolvingPaths(cachedSpecFiles: &cachedSpecFiles) var value = Set() return resolvedSpec.mergedDictionary(set: &value) @@ -118,19 +123,25 @@ public struct SpecFile { .reduce([:]) { $1.merged(onto: $0) }) } - func resolvingPaths(relativeTo basePath: Path = Path()) -> SpecFile { + func resolvingPaths(cachedSpecFiles: inout [Path: SpecFile], relativeTo basePath: Path = Path()) -> SpecFile { + if let cachedSpecFile = cachedSpecFiles[filePath] { + return cachedSpecFile + } + let relativePath = (basePath + self.relativePath).normalize() guard relativePath != Path() else { return self } let jsonDictionary = Project.pathProperties.resolvingPaths(in: self.jsonDictionary, relativeTo: relativePath) - return SpecFile( + let specFile = SpecFile( filePath: filePath, jsonDictionary: jsonDictionary, relativePath: self.relativePath, - subSpecs: subSpecs.map { $0.resolvingPaths(relativeTo: relativePath) } + subSpecs: subSpecs.map { $0.resolvingPaths(cachedSpecFiles: &cachedSpecFiles, relativeTo: relativePath) } ) + cachedSpecFiles[filePath] = specFile + return specFile } } diff --git a/Sources/ProjectSpec/SpecLoader.swift b/Sources/ProjectSpec/SpecLoader.swift index d4493891a..555e13715 100644 --- a/Sources/ProjectSpec/SpecLoader.swift +++ b/Sources/ProjectSpec/SpecLoader.swift @@ -16,7 +16,8 @@ public class SpecLoader { } public func loadProject(path: Path, projectRoot: Path? = nil, variables: [String: String] = [:]) throws -> Project { - let spec = try SpecFile(path: path, variables: variables) + var cachedSpecFiles: [Path: SpecFile] = [:] + let spec = try SpecFile(filePath: path, basePath: projectRoot ?? path.parent(), cachedSpecFiles: &cachedSpecFiles, variables: variables) let resolvedDictionary = spec.resolvedDictionary() let project = try Project(basePath: projectRoot ?? spec.basePath, jsonDictionary: resolvedDictionary) diff --git a/Tests/Fixtures/legacy_paths_test/legacy_included_paths_test.yml b/Tests/Fixtures/legacy_paths_test/legacy_included_paths_test.yml index de8cce3c4..7156ff0f1 100644 --- a/Tests/Fixtures/legacy_paths_test/legacy_included_paths_test.yml +++ b/Tests/Fixtures/legacy_paths_test/legacy_included_paths_test.yml @@ -1,5 +1,8 @@ configFiles: IncludedConfig: config +include: + - path: legacy_paths_test/recursive_include.yml + relativePaths: false options: carthageBuildPath: carthage_build carthageExecutablePath: carthage_executable @@ -9,8 +12,6 @@ targets: platform: tvOS configFiles: Config: config - sources: - - source dependencies: - framework: Framework info: diff --git a/Tests/Fixtures/legacy_paths_test/recursive_include.yml b/Tests/Fixtures/legacy_paths_test/recursive_include.yml new file mode 100644 index 000000000..0da21bd99 --- /dev/null +++ b/Tests/Fixtures/legacy_paths_test/recursive_include.yml @@ -0,0 +1,4 @@ +targets: + IncludedTarget: + sources: + - source diff --git a/Tests/PerformanceTests/PerformanceTests.swift b/Tests/PerformanceTests/PerformanceTests.swift index 6d2c5a2c4..a241e3483 100644 --- a/Tests/PerformanceTests/PerformanceTests.swift +++ b/Tests/PerformanceTests/PerformanceTests.swift @@ -13,9 +13,12 @@ class GeneratedPerformanceTests: XCTestCase { let project = try Project.testProject(basePath: basePath) let specPath = basePath + "project.yaml" try dumpYamlDictionary(project.toJSONDictionary(), path: specPath) + var cachedSpecFiles: [Path: SpecFile] = [:] measure { - let spec = try! SpecFile(path: specPath, variables: ProcessInfo.processInfo.environment) + let spec = try! SpecFile(path: specPath, + cachedSpecFiles: &cachedSpecFiles, + variables: ProcessInfo.processInfo.environment) _ = spec.resolvedDictionary() } }