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

Use lld when building with mzcompose on macOS #30797

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Conversation

def-
Copy link
Contributor

@def- def- commented Dec 11, 2024

$ bin/mzcompose --find testdrive down && bin/mzcompose --find testdrive --dev run default
$ echo >> src/materialized/src/bin/materialized.rs
$ bin/mzcompose --find testdrive down && bin/mzcompose --find testdrive --dev run default
[...]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 7.35s

Before this PR:

$ bin/mzcompose --find testdrive down && bin/mzcompose --find testdrive --dev run default
$ echo >> src/materialized/src/bin/materialized.rs
$ bin/mzcompose --find testdrive down && bin/mzcompose --find testdrive --dev run default
[...]
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 6m 35s

@teskje Can you try if this works for you?

Independently of this, we should also bump the crosstools-ng and the software it pulls it to get parity with ci-builder: MaterializeInc/homebrew-crosstools#10

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@def- def- requested a review from teskje December 11, 2024 14:23
@def- def- requested a review from benesch December 11, 2024 14:36
@def- def- force-pushed the pr-lld branch 2 times, most recently from db231c3 to ddf6cf6 Compare December 11, 2024 14:51
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

This makes a ton of sense. Just one comment about limiting the -B argument to just lld.

@def- I'm inclined to say that if we go this route, we shouldn't have the homebrew-toolchains repo try to install lld into this sysroot. In theory the homebrew-toolchains are meant to be usable by folks outside Materialize and those folks might not want to be forced to install lld. And using lld downstream, as demonstrated here, looks pretty clean!

misc/python/materialize/xcompile.py Outdated Show resolved Hide resolved
Incremental compile time for bin/mzcompose improves from 6min28s to 13s on my system by using lld.
@def-
Copy link
Contributor Author

def- commented Dec 11, 2024

I'm inclined to say that if we go this route, we shouldn't have the homebrew-toolchains repo try to install lld into this sysroot.

Yes, absolutely. I'm not able to get that PR working properly though, and probably less importance.

@def- def- enabled auto-merge December 11, 2024 18:23
@def- def- requested a review from benesch December 11, 2024 18:23
Copy link
Member

@benesch benesch left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

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

Works like a charm!

@def- def- merged commit 79d8655 into MaterializeInc:main Dec 11, 2024
73 checks passed
@def- def- deleted the pr-lld branch December 11, 2024 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants