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.

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

os/mac/xcode: add fast path for Xcode version detection #16388

Merged
merged 1 commit into from
Dec 23, 2023

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Dec 23, 2023

At best, MacOS::Xcode.detect_version would take ~0.12s - and likely even slower in CI environments.

This PR improves this by about 10x (100x if MacOS.active_developer_dir has already been called somewhere), by finding and reading version.plist rather than a slow shell-out to xcodebuild.

On CLT-only installs, we skip xcodebuild execution entirely and go straight to clang version guessing. It's debatable whether MacOS::Xcode.version should really be returning anything for CLT-only installs, but I understand why it's been chosen to behave that way.

Copy link
Contributor

@apainintheneck apainintheneck left a comment

Choose a reason for hiding this comment

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

Code looks good. I was able to repeat the results and the performance improvement locally.

This is a just micro-optimization, right? It seems like the result of Xcode.detect_version always gets memoized.

Nit: The Xcode.detect_version method is getting a bit large. It might make sense to break it up a little more.

@Bo98
Copy link
Member Author

Bo98 commented Dec 23, 2023

This is a just micro-optimization, right? It seems like the result of Xcode.detect_version always gets memoized.

Sort of. Micro in the sense you probably won't notice it on most machines but large enough that you will see clearly it in a brew prof output. It will trigger in the parent brew process and in build.rb subprocess so any effects there are doubled (build.rb/postinstall.rb startup is something in particular I'd like to see improved), so a quarter of a second is a fair bit. You can multiply the effect further by considering brew test-bot runs from cold states for each command run, but that's minor compared to the overall run.

It's one of those things I saw in the brew prof graph, looked at the code and said "why are we doing that way?". We could probably delete the xcodebuild code entirely to be honest - this will also solve your large method concern.

@Bo98 Bo98 merged commit 3798cac into Homebrew:master Dec 23, 2023
31 checks passed
@Bo98 Bo98 deleted the fast-xcode-version branch December 23, 2023 03:09
@MikeMcQuaid
Copy link
Member

Nice work @Bo98!

@github-actions github-actions bot added the outdated PR was locked due to age label Jan 24, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants