Optimize bottlenecks #529

michaeleisel · 2020-03-11T21:54:51Z

Short description 📝

Optimizes a few of the big bottlenecks. I can split it up into smaller PRs if wanted.

Solution 📦

Replace high-level Swift ways of doing things, which in these cases cause significant overhead, with fast and low-level ways of doing things

pepicrft · 2020-03-12T15:52:38Z

CExts.c

@@ -0,0 +1,59 @@
+//


Could you remove the headers from the file to be consistent with the rest of the codebase?

pepicrft · 2020-03-12T15:53:45Z

Sources/XcodeProj/Extensions/String+md5.swift

- let MD5Calculator = MD5(message)
- let MD5Data = MD5Calculator.calculate()
+ private func toHex(_ char: UInt8) -> UInt8 {
+ if char < 10 {


Would you mind adding a comment explaining the rationale behind comparing char with 10?

CExts.c

pepicrft · 2020-03-12T15:55:29Z

Sources/XcodeProj/Extensions/String+md5.swift

- for c in MD5Data {
- MD5String += String(format: "%02x", c)
+ var md5: String {
+ guard let data = data(using: .utf8, allowLossyConversion: true) else { abort() }


Aborting with no message is a terrible user experience for the user. What about passing a string with a explanatory comment of what happened?

pepicrft · 2020-03-12T15:56:03Z

Sources/XcodeProj/Extensions/String+md5.swift

+ var md5: String {
+ guard let data = data(using: .utf8, allowLossyConversion: true) else { abort() }
+ return data.withUnsafeBytes { bufferPointer in
+ let buffer = malloc(Int(CC_MD5_DIGEST_LENGTH))!.assumingMemoryBound(to: UInt8.self)


The logic here is hard to read. To simplify the understanding and maintenance of it in the future, I'd add a brief explanation of what it does.

pepicrft

Thank you very much @michaeleisel for improving the performance of XcodeProj. Overall it looks good. I just left some minor comments that are worth addressing before merging the PR.

pepicrft · 2020-03-12T15:59:52Z

Also, would you mind adding an entry to the CHANGELOG?

michaeleisel · 2020-03-14T01:55:39Z

The big question I'd like to discuss is, how to deal with the .c file? SwiftPM doesn't support mixed-language projects, and other package managers have issues as well. This is the first non-Swift file, right?

Here are some options:

Translate the C to Swift where it makes extensive use of unsafe pointers. This may incur some performance overhead
Create a separate package, e.g. XcodeProjCExt, whose version is synced with that of XcodeProj. This is what I do for https://github.com/michaeleisel/ZippyJSON, which puts its non-Swift code in https://github.com/michaeleisel/ZippyJSONCFamily . This is a pain, but also a good long-term investment if it will be good to have more code like this in the future. XcodeProj is used in large projects where it has a large workload, so I think it could be worth it. Just as a thought experiment, if the whole project were written in C++ (which I am not actually suggesting), it could easily be 10x faster.
?

pepicrft · 2020-03-16T10:16:13Z

I lean more towards the second option, having an independent package with those extensions. I added you to the contributors team, which has access to this repository that I just created to host that package. Feel free to push the extension there and the Package.swift to define the dependency from this PR.

Thanks a lot @michaeleisel for bringing this improvement. Performance is something we've disregarded for a long time and it's great having your experience with C and your contributions to make XcodeProj as fast as possible.

michaeleisel · 2020-03-16T13:38:48Z

🎉

michaeleisel · 2020-03-16T13:57:24Z

just pushed the package. note that the first branch i pushed was not master, it was me-init. so, that's now the default branch, which you may want to change in the settings

michaeleisel · 2020-03-16T13:58:17Z

also, now that we're using C and unsafe Swift, in case we're not already, it would be good to run unit tests pre-release with:

address sanitizer
thread sanitizer
memory leaks
guard malloc

michaeleisel · 2020-03-16T13:59:57Z

Sources/XcodeProj/Extensions/String+md5.swift

- var MD5String = String()
- for c in MD5Data {
- MD5String += String(format: "%02x", c)
+ var md5: String {


i can rewrite the meat of this function in C as well

it would be faster (swift is not very good at stack allocation of primitive arrays

Agree. If you think it'll be faster let's go for it!

pepicrft · 2020-03-16T14:24:36Z

😛Just changed it to be master the default. Would you mind pushing your changes to master?

pepicrft · 2020-03-16T14:25:10Z

also, now that we're using C and unsafe Swift, in case we're not already, it would be good to run unit tests pre-release with:

Haven't done it before. Is there any flag that we can pass to the SPM to run the tests with those things enabled? Feel free to adjust the GitHub Actions workflow as needed.

michaeleisel · 2020-03-16T18:32:45Z

ptal. fyi, i'm having issues with doing .package(url: "https://github.com/tuist/XcodeProjCExt", .exact("7.8.0")) (although .package(path: "../XcodeProjCExt") works). i get "invalidAccess". any ideas?

michaeleisel · 2020-03-16T18:34:50Z

it looks like the github workflow doesn't do any unit tests? are unit tests run in some other way, pre-release?

michaeleisel · 2020-03-19T21:57:28Z

Got the invalidAccess thing fixed (just a cache issue). For swiftpm you can add --sanitize=<sanitizer>, e.g. swift test --sanitize=address

pepicrft · 2020-03-20T11:20:56Z

Are we good to merge @michaeleisel ?

michaeleisel · 2020-03-20T16:08:17Z

i have one more change i want to make, but i want to make sure it doesn't cause any perf regressions. are there are any perf tests, preferably end-to-end?

pepicrft · 2020-03-21T08:21:01Z

i have one more change i want to make, but i want to make sure it doesn't cause any perf regressions. are there are any perf tests, preferably end-to-end?

We don't have at the moment. Do you want to add them as part of this PR?

michaeleisel · 2020-03-26T23:11:31Z

OK it looks good to me now. No perf tests, but that could be for another day

michaeleisel · 2020-04-01T19:28:23Z

Are we good to merge?

changes

bbcfda0

pepicrft reviewed Mar 12, 2020

View reviewed changes

CExts.c Outdated Show resolved Hide resolved

pepicrft reviewed Mar 12, 2020

View reviewed changes

CExts.c Outdated Show resolved Hide resolved

pepicrft reviewed Mar 12, 2020

View reviewed changes

pepicrft requested changes Mar 12, 2020

View reviewed changes

michaeleisel commented Mar 16, 2020

View reviewed changes

michaeleisel mentioned this pull request Mar 16, 2020

Optimize bottlenecks kylef/PathKit#72

Closed

michaeleisel added 4 commits March 16, 2020 14:27

updates

018d559

revert

1969495

clean up

34353a2

commit

1f42506

changelog

53b83a5

update Package.resolved

71521f3

pepicrft approved these changes Mar 20, 2020

View reviewed changes

update xcodegencext

9d53df0

michaeleisel added 2 commits March 20, 2020 11:48

cleanup

5bf5654

cleanup

c661ffe

better string API

fe446c5

pepicrft merged commit b722cee into tuist:master Apr 2, 2020

elliottwilliams mentioned this pull request Nov 7, 2020

Linux support #585

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize bottlenecks #529

Optimize bottlenecks #529

michaeleisel commented Mar 11, 2020

pepicrft Mar 12, 2020

michaeleisel Mar 14, 2020

pepicrft Mar 12, 2020

pepicrft Mar 12, 2020

pepicrft Mar 12, 2020

pepicrft left a comment

pepicrft commented Mar 12, 2020

michaeleisel commented Mar 14, 2020

pepicrft commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 16, 2020 •

edited

Loading

michaeleisel Mar 16, 2020

michaeleisel Mar 16, 2020

pepicrft Mar 16, 2020

pepicrft commented Mar 16, 2020

pepicrft commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 19, 2020

pepicrft commented Mar 20, 2020

michaeleisel commented Mar 20, 2020

pepicrft commented Mar 21, 2020

michaeleisel commented Mar 26, 2020

michaeleisel commented Apr 1, 2020

Optimize bottlenecks #529

Optimize bottlenecks #529

Conversation

michaeleisel commented Mar 11, 2020

Short description 📝

Solution 📦

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

pepicrft left a comment

Choose a reason for hiding this comment

pepicrft commented Mar 12, 2020

michaeleisel commented Mar 14, 2020

pepicrft commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepicrft commented Mar 16, 2020

pepicrft commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 16, 2020

michaeleisel commented Mar 19, 2020

pepicrft commented Mar 20, 2020

michaeleisel commented Mar 20, 2020

pepicrft commented Mar 21, 2020

michaeleisel commented Mar 26, 2020

michaeleisel commented Apr 1, 2020

michaeleisel commented Mar 16, 2020 •

edited

Loading