-
-
Notifications
You must be signed in to change notification settings - Fork 303
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.
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
Optimize bottlenecks #529
Optimize bottlenecks #529
Conversation
CExts.c
Outdated
@@ -0,0 +1,59 @@ | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you remove the headers from the file to be consistent with the rest of the codebase?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
let MD5Calculator = MD5(message) | ||
let MD5Data = MD5Calculator.calculate() | ||
private func toHex(_ char: UInt8) -> UInt8 { | ||
if char < 10 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind adding a comment explaining the rationale behind comparing char with 10?
for c in MD5Data { | ||
MD5String += String(format: "%02x", c) | ||
var md5: String { | ||
guard let data = data(using: .utf8, allowLossyConversion: true) else { abort() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aborting with no message is a terrible user experience for the user. What about passing a string with a explanatory comment of what happened?
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Also, would you mind adding an entry to the CHANGELOG? |
The big question I'd like to discuss is, how to deal with the Here are some options:
|
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 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. |
🎉 |
just pushed the package. note that the first branch i pushed was not master, it was |
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:
|
var MD5String = String() | ||
for c in MD5Data { | ||
MD5String += String(format: "%02x", c) | ||
var md5: String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i can rewrite the meat of this function in C as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be faster (swift is not very good at stack allocation of primitive arrays
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. If you think it'll be faster let's go for it!
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. |
ptal. fyi, i'm having issues with doing |
it looks like the github workflow doesn't do any unit tests? are unit tests run in some other way, pre-release? |
Got the |
Are we good to merge @michaeleisel ? |
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? |
OK it looks good to me now. No perf tests, but that could be for another day |
Are we good to merge? |
Short description 📝
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