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

Prevent Building Release On X86 without SSE3 #3485

Closed
tustvold opened this issue Jan 7, 2023 · 10 comments
Closed

Prevent Building Release On X86 without SSE3 #3485

tustvold opened this issue Jan 7, 2023 · 10 comments
Labels
development-process Related to development process of arrow-rs enhancement Any new improvement worthy of a entry in the changelog

Comments

@tustvold
Copy link
Contributor

tustvold commented Jan 7, 2023

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

I was messing around with simd-json and noticed that it fails to compile if there is no SIMD target_feature set, and wondered if we wanted to do something similar.

Specifically I am thinking of returning an error if compiling in release mode for x86 without sse3 enabled. These instructions were first introduced in 2004 and all computers in the latest steam hardware survey support them. They are, however, not enabled by default.

Describe the solution you'd like

Add the following to arrow-buffer

#[cfg(all(target_arch = "x86_64", not(target_feature = "sse3")))]
fn please_compile_with_simd_target_feature() -> ! {} // E.g. RUSTFLAGS="-C target-cpu=haswell"

If the user compiles without any options set they will get

error[E0308]: mismatched types
  --> arrow-buffer/src/lib.rs:21:49
   |
21 | fn please_compile_with_simd_target_feature() -> ! {} // E.g. RUSTFLAGS="-C target-cpu=haswell"
   |    ---------------------------------------      ^ expected `!`, found `()`
   |    |
   |    implicitly returns `()` as its body has no tail or `return` expression
   |
   = note:   expected type `!`
           found unit type `()`

For more information about this error, try `rustc --explain E0308`.

The UX isn't fantastic, but I think it is better than what currently happens which is people silently get poor out of the box performance.

Describe alternatives you've considered

We could not do this

Additional context

Tagging a few people as this will have downstream implications

@alamb @andygrove @viirya @thinkharderdev @jhorstmann @Dandandan

@tustvold tustvold added the enhancement Any new improvement worthy of a entry in the changelog label Jan 7, 2023
@tustvold
Copy link
Contributor Author

tustvold commented Jan 7, 2023

I plan to send something out to the mailing list also

@thinkharderdev
Copy link
Contributor

Wouldn't this also fail if compiled with something like -C target-feature=+avx

@tustvold
Copy link
Contributor Author

tustvold commented Jan 7, 2023

We could easily make it only error if none of sse3, ssse3, avx, avx2, etc... are specified. Although I'm not aware of any CPUs that support AVX and not SSE3, so only specifying +avx seems unfortunate

@alamb
Copy link
Contributor

alamb commented Jan 8, 2023

I somewhat worry about the UX of making this an error. Specifically:

  1. I am a new user that is just trying arrow out the first time
  2. I try my normal work flow of cargo run and I get some (to me) esoteric error about instruction sets and compiler flags

Or

  1. I am a long time user and happy with performance
  2. I upgrade to the latest version
  3. Now by project fails to build and I have to go spend time debugging what is wrong

What if we started with a warning (rather than error) -- that would help guide people to the correct usage but wouldn't increase the startup cost of using the project for most people.

If we went with a warning maybe we could also increase the default desired instructions too (like to SSE4.2 as suggested by @pitrou on the mailing list)

@tustvold
Copy link
Contributor Author

tustvold commented Jan 8, 2023

A warning would be ideal, however, I'm not aware of a way to generate warnings in such a way that Cargo won't suppress them...

@jhorstmann
Copy link
Contributor

Agree that a warning would be preferrable and that we should strongly encourage users to specify a more modern target cpu. Maybe something like this could be done inside a build.rs where we could detect the actually available cpu features?

As a baseline, I think SSE3 offers too little on top of the SSE2 to be worth the inevitable support questions. Maybe you meant SSSE3 instead, that introduced the pshufb instruction which is useful for many byte-based operations for example in parsing, but I doubt arrow-rs itself would make use of that. SSE4.1 introduced some useful functionality, but I think a big jump in performance can only be expected by going directly to AVX or AVX512 because of their wider vectors. Unfortunately AVX is still not as ubiquitous, mainly because of Intel Atom CPUs.

@tustvold
Copy link
Contributor Author

tustvold commented Jan 8, 2023

Maybe you meant SSSE3 instead

I just meant something that isn't the default to force the user to specify something, most likely AVX2, but not prescribing anything

@tustvold
Copy link
Contributor Author

tustvold commented Jan 18, 2023

Sadly there does not appear to be a mechanism to unconditionally emit a warning - rust-lang/cargo#3777

Edit: I've filed - rust-lang/cargo#11593

@bjchambers
Copy link
Contributor

It would also be good to have some kind of benchmark showing the improvement this makes.

As a user, I would get frustrated if it didn't work, and after jumping through the hoops it only gives me 5% in some cases. If it regularly gave me a decent improvement, then it would be much more compelling.

Do we have a sense of how much better turning on SSE3 or others would be? Another option could be to make that more prominent in the getting started guide. If I was reading "how to get started" and saw I could easily get a win in performance, I'd be likely to remember to set that up...

@tustvold
Copy link
Contributor Author

tustvold commented Feb 3, 2023

Do we have a sense of how much better turning on SSE3 or others would be

It depends on the kernels, the biggest win is for the comparison kernels where SIMD yields 5x performance improvements for integer comparisons. Other kernels are significantly less pronounced, with arithmetic only showing a 20% performance uplift. So it really depends...

I think I've been convinced returning a hard error is not a good idea, hopefully progress can be made on rust-lang/cargo#11593 to allow a suggestion... Otherwise I guess we just have to rely on people reading the crates.io docs

@tustvold tustvold closed this as not planned Won't fix, can't repro, duplicate, stale Mar 16, 2023
@tustvold tustvold added the development-process Related to development process of arrow-rs label Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of arrow-rs enhancement Any new improvement worthy of a entry in the changelog
Projects
None yet
Development

No branches or pull requests

5 participants