Apply cpu-optimisation to Rust projects #15544

Tokarak · 2023-06-13T10:46:08Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

Library/Homebrew/formula.rb

This reverts commit c344a3f.

Tokarak · 2023-06-13T18:41:46Z

My worry is that conditionally setting target-cpu via command line could start overwriting build caches (if homebrew uses them). This should be fine if there are no caches (:Fr:), or different cache folders based on version. Otherwise rust-lang/cargo#7773 is a problem.

Tokarak · 2023-06-13T18:53:45Z

Any idea how to test?

MikeMcQuaid

Nice work so far!

MikeMcQuaid · 2023-06-14T12:29:16Z

Library/Homebrew/extend/os/mac/hardware.rb

+  # Override
+  # Mirrors version-dependent logic of oldest_cpu
+  sig { params(version: T.nilable(Version)).returns(String) }
+  def self.rustflags_target_cpu(version = nil)


When would version be passed through here rather than just use the current MacOS.version?

Library/Homebrew/extend/os/mac/hardware.rb

carlocab · 2023-06-14T15:06:22Z

Library/Homebrew/extend/os/mac/hardware.rb

+      MacOS.version
+    end
+    if Hardware::CPU.intel? && version >= :mojave
+      "'-Ctarget-cpu=nehalem'"


Suggested change

"'-Ctarget-cpu=nehalem'"

"--codegen target-cpu=nehalem"

Let callers handle the quoting, if it's needed.

Use the long version of the flag.

I'd still rather use oldest_cpu here instead of hard-coding nehalem.

carlocab · 2023-06-14T15:08:06Z

Library/Homebrew/formula.rb

@@ -1543,7 +1543,8 @@ def std_configure_args
  # Standard parameters for cargo builds.
  sig { params(root: T.any(String, Pathname), path: String).returns(T::Array[T.any(String, Pathname)]) }
  def std_cargo_args(root: prefix, path: ".")
-    ["--locked", "--root", root, "--path", path]
+    ["--locked", "--root", root, "--path", path, "--config",
+     "build.rustflags=[#{Hardware.rustflags_target_cpu}]"]


How does this interact with RUSTFLAGS set in the environment? Does it override them? Append to them?

I think we may prefer to set RUSTFLAGS in the environment as well as/instead of doing this, since not all rustc invocations happen through a cargo call that uses std_cargo_args. See, for example, gstreamer, or the many formulae that have a cryptography resource.

Tokarak · 2023-06-14T16:26:15Z

OK — new, prefered method: set RUSTFLAGS through a environment variable.

I think I'll add it to std env for now. I'm currently sanitising and overriding RUSTFLAGS.

Should this be in the super environment?
Should I prepend, append, or overwrite (thus sanitise)?
Is there a envvar for the target macos version, or does MacOs.version handle that?
Will there be an issue with caching, as conditional rustflags could start overwriting any compiled crates which had the opposite flag?
Testing!

Current review suggestions:

long option

 "-Ctarget-cpu=nehalem" ===>>>
 "--codegen target-cpu=nehalem"

Refactor rustflags_target_cpu to use oldest_cpu

Since Rust already maximins the cpu features, it's best to leave it to it. I think that a single hard-coding, 10 lines away from the other hardcoding is much less of a maintainance problem than an unsemantic dictionary (arm_vortex_tempest => 'apple-m1' ???) (think of the room for error too — what if oldest_cpu starts returning powerpc cpus, which are not available on any darwin target-triples. Plus nehalem is hardcoded in things like requires_nehalem?.
If you must insist, what is the goal of avoiding hardcoding? Is this way unsatisfactory?

carlocab · 2023-06-16T17:02:33Z

Should this be in the super environment?

Yes.

Should I prepend, append, or overwrite (thus sanitise)?

Do the same thing as the other similar flags (e.g. CFLAGS), which I believe is overwrite.

Is there a envvar for the target macos version, or does MacOs.version handle that?

You can try HOMEBREW_MACOS_VERSION_NUMERIC but rely on MacOS.version where you can.

Will there be an issue with caching, as conditional rustflags could start overwriting any compiled crates which had the opposite flag?

Maybe, but cargo builds inside brew live inside HOMEBREW_CACHE so it shouldn't affect user usage of cargo.

Refactor rustflags_target_cpu to use oldest_cpu

Since Rust already maximins the cpu features, it's best to leave it to it. I think that a single hard-coding, 10 lines away from the other hardcoding is much less of a maintainance problem than an unsemantic dictionary (arm_vortex_tempest => 'apple-m1' ???) (think of the room for error too — what if oldest_cpu starts returning powerpc cpus, which are not available on any darwin target-triples. Plus nehalem is hardcoded in things like requires_nehalem?.
If you must insist, what is the goal of avoiding hardcoding? Is this way unsatisfactory?

Because having the same constants in multiple places (even when it they're not far away from each other) just isn't a good idea. The hypothetical situations you're worried about haven't arisen yet, and I don't think they will. If they do, then we can handle it then.

Tokarak · 2023-06-17T09:41:58Z

I now use oldest_cpu. I still fallback to rust defaults where I can.

carlocab · 2023-06-17T10:18:04Z

Library/Homebrew/hardware.rb

+    sig { returns(String) }
+    def rustflags_target_cpu
+      CPU.rust_optimisation_flags.fetch(oldest_cpu, "")


Suggested change

sig { returns(String) }

def rustflags_target_cpu

CPU.rust_optimisation_flags.fetch(oldest_cpu, "")

sig { returns(T.nilable(String)) }

def rustflags_target_cpu

CPU.rust_optimisation_flags.fetch(oldest_cpu, nil)

Also: we should try to support users doing something like

brew install --build-bottle --bottle-arch=ivybridge

which this doesn't currently do.

carlocab · 2023-06-17T10:18:37Z

Library/Homebrew/hardware.rb

+          native:  "-Ctarget-cpu=native",
+          nehalem: "-Ctarget-cpu=nehalem",


Suggested change

native: "-Ctarget-cpu=native",

nehalem: "-Ctarget-cpu=nehalem",

native: "--codegen target-cpu=native",

nehalem: "--codegen target-cpu=nehalem",

Please use the long flags for better readability.

carlocab · 2023-06-17T10:26:53Z

Library/Homebrew/hardware.rb

+      # Only give values where it is an improvement over rust cpu defaults
+      # Rust already defaults to the oldest supported cpu for each target-triple
+      # Including apple-m1 since Rust 1.71 for aarch64-apple-darwin.


I'd really rather be explicit here rather than bake in assumptions about the behaviour of rustc. None of us track the development of rustc that carefully, so if they decide to bump the minimum of core2 before we do, then this method will no longer behave as expected, and that's likely to go unnoticed for some time.

The reverse (i.e. if we decide to bump the minimum of core2 before Rust does) is also an issue, albeit a smaller one.

Library/Homebrew/hardware.rb

MikeMcQuaid

Looks good so far!

Library/Homebrew/hardware.rb

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

Tokarak · 2023-06-20T23:06:25Z

Looks like the CI failure is due to https://http://sequelpro.com/ being down.

MikeMcQuaid

Looks good to me:

if it works for you in local testing
once @carlocab is also ✅

Tokarak · 2023-06-21T18:35:26Z

brew tests --changed is fine.

carlocab · 2023-06-22T11:11:41Z

I'm currently neck-deep in Homebrew/homebrew-core#134251, so I haven't had a chance to take a proper look here. Will come back to it next week.

Library/Homebrew/hardware.rb

Co-authored-by: Alexander Bayandin <alexander@bayandin.dev>

carlocab

Thanks; I'm happy with how this is looking currently.

Bonus points for the following (but you can save it for a future PR):

Support for brew install --build-bottle --bottle-arch=<custom-arch>
An ENV.rustflags accessor, just like what we have for ENV.cflags, ENV.ldflags, etc.

MikeMcQuaid

Thanks so much for your first contribution (hopefully of many)! Without people like you submitting PRs we couldn't run this project. You rock, @Tokarak!

BugenZhao · 2023-07-09T05:14:48Z

Hi, all. According to The Cargo Book, setting the RUSTFLAGS environment variable will override any RUSTFLAGS specified in a cargo package's config file.

This may cause some packages to fail to compile if they're using RUSTFLAGS to enable some features, like tokio_unstable. For example, RisingWave relies on tokio's several unstable features and users find it fail to build after upgrading brew to 4.0.27.

https://github.com/risingwavelabs/risingwave/blob/f03dbc62c4e07fd16164a6b81219d3ffd5f86fa9/.cargo/config.toml#L32-L36
risingwavelabs/homebrew-risingwave#10
https://docs.rs/tokio/latest/tokio/task/fn.consume_budget.html

carlocab · 2023-07-10T10:07:39Z

I suggest doing

ENV.delete "RUSTFLAGS"

in the install method of your formula if you need to set build.rustflags in your config.toml.

Apply cpu-optimisation to Rust projects (hardcoded)

c344a3f

Tokarak force-pushed the cargo_cpu branch from 42cd342 to c344a3f Compare June 13, 2023 10:50

MikeMcQuaid reviewed Jun 13, 2023

View reviewed changes

Library/Homebrew/formula.rb Outdated Show resolved Hide resolved

Tokarak added 4 commits June 13, 2023 19:17

Revert "Apply cpu-optimisation to Rust projects (hardcoded)"

a072508

This reverts commit c344a3f.

Add rustflags_target_cpu method

066a888

Use rustflags_target_cpu

b4dd639

Remove FIXMEs

d4bfbb1

Fix

2bd20ea

Tokarak marked this pull request as ready for review June 13, 2023 18:53

Fix error on empty target-cpu

c698174

MikeMcQuaid mentioned this pull request Jun 14, 2023

Optimise rust-based formulas by setting target-cpu #15530

Closed

1 task

MikeMcQuaid reviewed Jun 14, 2023

View reviewed changes

Tokarak force-pushed the cargo_cpu branch from 5401cbf to c698174 Compare June 14, 2023 14:14

carlocab reviewed Jun 14, 2023

View reviewed changes

Tokarak added 3 commits June 14, 2023 16:46

Line break

cccad3e

Prepare to migrate to RUSTFLAGS

261f55a

Set target-cpu through RUSTFLAGS

2133da8

Tokarak added 2 commits June 17, 2023 10:16

Add RUSTFLAGS to ENV/super

9a99b93

Refactor to use oldest_cpu

9a19f5b

Style changes

73c339d

carlocab reviewed Jun 17, 2023

View reviewed changes

More cpus in rust_optimisation_flags

40aa419

MikeMcQuaid reviewed Jun 19, 2023

View reviewed changes

Library/Homebrew/hardware.rb Outdated Show resolved Hide resolved

MikeMcQuaid reviewed Jun 19, 2023

View reviewed changes

Library/Homebrew/hardware.rb Outdated Show resolved Hide resolved

Library/Homebrew/hardware.rb Outdated Show resolved Hide resolved

Library/Homebrew/hardware.rb Outdated Show resolved Hide resolved

Improve logic (code review)

87158ac

Co-authored-by: Mike McQuaid <mike@mikemcquaid.com>

brew style --fix

55bdbab

MikeMcQuaid requested a review from carlocab June 21, 2023 11:37

MikeMcQuaid approved these changes Jun 21, 2023

View reviewed changes

bayandin reviewed Jun 23, 2023

View reviewed changes

Library/Homebrew/hardware.rb Outdated Show resolved Hide resolved

Wording

521fdcb

Co-authored-by: Alexander Bayandin <alexander@bayandin.dev>

carlocab approved these changes Jul 1, 2023

View reviewed changes

MikeMcQuaid approved these changes Jul 4, 2023

View reviewed changes

MikeMcQuaid merged commit fd4f488 into Homebrew:master Jul 4, 2023

BugenZhao mentioned this pull request Jul 9, 2023

"brew install risingwave" Failed risingwavelabs/homebrew-risingwave#10

Closed

github-actions bot added the outdated PR was locked due to age label Aug 10, 2023

github-actions bot locked as resolved and limited conversation to collaborators Aug 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Apply cpu-optimisation to Rust projects #15544

Apply cpu-optimisation to Rust projects #15544

Tokarak commented Jun 13, 2023 •

edited

Loading

Tokarak commented Jun 13, 2023

Tokarak commented Jun 13, 2023

MikeMcQuaid left a comment

MikeMcQuaid Jun 14, 2023

carlocab Jun 14, 2023

carlocab Jun 14, 2023

Tokarak commented Jun 14, 2023 •

edited

Loading

carlocab commented Jun 16, 2023

Tokarak commented Jun 17, 2023

carlocab Jun 17, 2023

carlocab Jun 17, 2023

carlocab Jun 17, 2023

MikeMcQuaid left a comment

Tokarak commented Jun 20, 2023

MikeMcQuaid left a comment

Tokarak commented Jun 21, 2023

carlocab commented Jun 22, 2023

carlocab left a comment

MikeMcQuaid left a comment

BugenZhao commented Jul 9, 2023 •

edited

Loading

carlocab commented Jul 10, 2023

		native: "-Ctarget-cpu=native",
		nehalem: "-Ctarget-cpu=nehalem",

Apply cpu-optimisation to Rust projects #15544

Apply cpu-optimisation to Rust projects #15544

Conversation

Tokarak commented Jun 13, 2023 • edited Loading

Tokarak commented Jun 13, 2023

Tokarak commented Jun 13, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid Jun 14, 2023

Choose a reason for hiding this comment

carlocab Jun 14, 2023

Choose a reason for hiding this comment

carlocab Jun 14, 2023

Choose a reason for hiding this comment

Tokarak commented Jun 14, 2023 • edited Loading

carlocab commented Jun 16, 2023

Tokarak commented Jun 17, 2023

carlocab Jun 17, 2023

Choose a reason for hiding this comment

carlocab Jun 17, 2023

Choose a reason for hiding this comment

carlocab Jun 17, 2023

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Tokarak commented Jun 20, 2023

MikeMcQuaid left a comment

Choose a reason for hiding this comment

Tokarak commented Jun 21, 2023

carlocab commented Jun 22, 2023

carlocab left a comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

BugenZhao commented Jul 9, 2023 • edited Loading

carlocab commented Jul 10, 2023

Tokarak commented Jun 13, 2023 •

edited

Loading

Tokarak commented Jun 14, 2023 •

edited

Loading

BugenZhao commented Jul 9, 2023 •

edited

Loading