Optimize bottlenecks #803

michaeleisel · 2020-03-11T22:11:13Z

These are some bottlenecks that were causing significant performance overhead for me. The changes are rough, but if they look good overall I can clean them up

michaeleisel · 2020-03-11T22:11:26Z

Sources/ProjectSpec/Decoding.swift

@@ -4,18 +4,36 @@ import PathKit
 import Yams

 extension Dictionary where Key: JSONKey {
-
-    public func json<T: NamedJSONDictionaryConvertible>(atKeyPath keyPath: JSONUtilities.KeyPath, invalidItemBehaviour: InvalidItemBehaviour<T> = .remove) throws -> [T] {
+    public func json<T: NamedJSONDictionaryConvertible>(atKeyPath keyPath: JSONUtilities.KeyPath, invalidItemBehaviour: InvalidItemBehaviour<T> = .remove, parallel: Bool = false) throws -> [T] {


parallelizing the json parsing

fyi if we used zippyjson for the json parsing, even single-threaded it would be a lot (maybe 10x?) faster. but parallelizing is a simple win for this commit

michaeleisel · 2020-03-11T22:11:49Z

Sources/XcodeGenKit/CarthageDependencyResolver.swift

@@ -10,11 +10,14 @@ import ProjectSpec
 import PathKit

 public class CarthageDependencyResolver {


parallelizing dependency resolution

ghost · 2020-03-13T09:40:07Z

Sources/ProjectSpec/Decoding.swift

+                    let jsonDictionary: JSONDictionary = try! dictionary.json(atKeyPath: .key(key))
+                    let item = try! T(name: key, jsonDictionary: jsonDictionary)


no need to use exclamation mark on try

agreed. any other concerns?

nope. but bear in mind i am not a real reviewer 🙈

According to another paring function, it would be better to set try, not try!

XcodeGen/Sources/ProjectSpec/Decoding.swift

Line 39 in a0e144b

public func json<T: NamedJSONConvertible>(atKeyPath keyPath: JSONUtilities.KeyPath, invalidItemBehaviour: InvalidItemBehaviour<T> = .remove) throws -> [T] {

This is also because paring error should be handled on here

XcodeGen/Sources/ProjectSpec/Project.swift

Line 166 in a0e144b

public init(basePath: Path = "", jsonDictionary: JSONDictionary) throws {

looking at this further, i think it would be simpler to use try! instead of try. the issue is that these blocks are being run asynchronously, so they don't just bubble up to this function's caller. there are other strategies, like storing a result type in the list and then if any of the results are failures, throw the first failure as the error out of this function. we can do that if people want though

eh, i'll just do it with try actually

michaeleisel · 2020-03-25T15:27:21Z

Is this project still maintained?

yonaskolb · 2020-03-26T08:51:32Z

Thanks for your work here @michaeleisel. Would you be able to post some timings before and after this PR (after a rebase) for a large project, and also for the performance tests. Thanks!

michaeleisel · 2020-03-26T13:27:20Z

Sources/XcodeGenKit/CarthageVersionLoader.swift

 class CarthageVersionLoader {

    private let buildPath: Path
-    private var cachedFiles: [String: CarthageVersionFile] = [:]
+    private var cachedFilesMutex: Mutex<[String: CarthageVersionFile]> = Mutex([:])


note that we'll need to be careful in the future not to add any non-thread-safe state to CarthageVersionLoader. for now it's just cachedFiles that needs this, but something to keep in mind

Let's add a comment to CarthageVersionLoader to warn future us.

michaeleisel · 2020-03-26T15:58:53Z

For carthage dependencies, on my 8 (physical) core machine it reduces the time from 600ms to 200ms for a large project. For the JSON parsing, it goes from 1,500 ms to 300ms. for performance testing, i can take a look. but first, are we good with these at a high level? (i.e., excluding stuff like try vs. try!, which can easily be fixed)

brentleyjones · 2020-03-26T16:04:24Z

I can't speak for @yonaskolb, but I'm interested in this. From a cursory glance I'm good with it at a high level.

michaeleisel · 2020-03-26T19:18:23Z

ok i'm happy with it now personally as well. here are the perf test results: https://gist.github.com/michaeleisel/3d1a5316d233571ca5e2ec9531351c82

michaeleisel · 2020-04-01T19:27:54Z

are we good to merge?

brentleyjones

Seems our project doesn't benefit much from this (not to say it's not a good change).

The spike at the start is this change. The spikes at the end are from tuist/XcodeProj#529.

Can you add a CHANGELOG entry? I think this is good to be merged.

brentleyjones · 2020-04-03T13:51:37Z

Sources/XcodeGenKit/CarthageVersionLoader.swift

 class CarthageVersionLoader {

    private let buildPath: Path
-    private var cachedFiles: [String: CarthageVersionFile] = [:]
+    private var cachedFilesMutex: Mutex<[String: CarthageVersionFile]> = Mutex([:])


Let's add a comment to CarthageVersionLoader to warn future us.

brentleyjones · 2020-04-03T14:08:10Z

More details on our performance bottleneck:

Seems it's all in project generation and writing. In particular these two functions are hot spots:

XcodeGen/Sources/XcodeGenKit/SourceGenerator.swift

Lines 80 to 82 in 01c6959

    
           func getAllSourceFiles(targetType: PBXProductType, sources: [TargetSource]) throws -> [SourceFile] { 
        
               try sources.flatMap { try getSourceFiles(targetType: targetType, targetSource: $0, path: project.basePath + $0.path) } 
        
           }

(or just https://github.com/yonaskolb/XcodeGen/blob/master/Sources/XcodeGenKit/PBXProjGenerator.swift#L222)

and

https://github.com/tuist/XcodeProj/blob/master/Sources/XcodeProj/Utils/ReferenceGenerator.swift#L27

michaeleisel · 2020-04-03T16:11:37Z

Yeah, I've only ensured it improves my own use case. I spent some time with that getAllSourceFiles. Without a deep understanding, it seems somewhat difficult to optimize. Maybe we could open another issue about it.

michaeleisel · 2020-04-03T16:15:20Z

@brentleyjones ptal

michaeleisel commented Mar 11, 2020

View reviewed changes

ghost reviewed Mar 13, 2020

View reviewed changes

brentleyjones requested a review from yonaskolb March 16, 2020 17:22

commit

df8a873

michaeleisel commented Mar 26, 2020

View reviewed changes

fixups

e5f07f5

michaeleisel force-pushed the me-rm-bot branch from a0e144b to e5f07f5 Compare March 26, 2020 19:18

yonaskolb mentioned this pull request Mar 26, 2020

Performance regression since 2.2.0 #628

Open

brentleyjones assigned brentleyjones and yonaskolb and unassigned brentleyjones Apr 1, 2020

brentleyjones reviewed Apr 3, 2020

View reviewed changes

michaeleisel added 2 commits April 3, 2020 12:14

pr

88d3b44

fix

88831bf

brentleyjones approved these changes Apr 3, 2020

View reviewed changes

brentleyjones merged commit c3693d4 into yonaskolb:master Apr 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize bottlenecks #803

Optimize bottlenecks #803

michaeleisel commented Mar 11, 2020

michaeleisel Mar 11, 2020

michaeleisel Mar 26, 2020

michaeleisel Mar 11, 2020

ghost Mar 13, 2020

michaeleisel Mar 13, 2020

ghost Mar 17, 2020

freddi-kit Mar 26, 2020 •

edited

Loading

michaeleisel Mar 26, 2020

michaeleisel Mar 26, 2020

michaeleisel commented Mar 25, 2020

yonaskolb commented Mar 26, 2020

michaeleisel Mar 26, 2020

brentleyjones Apr 3, 2020

michaeleisel commented Mar 26, 2020

brentleyjones commented Mar 26, 2020

michaeleisel commented Mar 26, 2020

michaeleisel commented Apr 1, 2020

brentleyjones left a comment

brentleyjones Apr 3, 2020

brentleyjones commented Apr 3, 2020

michaeleisel commented Apr 3, 2020

michaeleisel commented Apr 3, 2020

		@@ -10,11 +10,14 @@ import ProjectSpec
		import PathKit

		public class CarthageDependencyResolver {

		let jsonDictionary: JSONDictionary = try! dictionary.json(atKeyPath: .key(key))
		let item = try! T(name: key, jsonDictionary: jsonDictionary)

Optimize bottlenecks #803

Optimize bottlenecks #803

Conversation

michaeleisel commented Mar 11, 2020

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

freddi-kit Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeleisel commented Mar 25, 2020

yonaskolb commented Mar 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeleisel commented Mar 26, 2020

brentleyjones commented Mar 26, 2020

michaeleisel commented Mar 26, 2020

michaeleisel commented Apr 1, 2020

brentleyjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentleyjones commented Apr 3, 2020

michaeleisel commented Apr 3, 2020

michaeleisel commented Apr 3, 2020

freddi-kit Mar 26, 2020 •

edited

Loading