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

Enable building with non-nightly complier #205

Closed
wants to merge 1 commit into from

Conversation

hsivonen
Copy link
Member

A recent compiler change removed support for compiling the simd crate. As a result, it seems that Firefox needs to migrate to packed_simd before packed_simd becomes core::simd.

The Firefox build system previously had problems with setting RUSTC_BOOTSTRAP=1 on the build system level and, as a result, migrated to relying on individual crates setting RUSTC_BOOTSTRAP=1 in their build.rs (first in order to use a custom allocator, but at present the only use case is the simd crate).

I believe this PR makes packed_simd compatible with the Firefox build system.

@hsivonen
Copy link
Member Author

CC @glandium who made the Firefox build system change.

@hsivonen
Copy link
Member Author

Looks like the Travis failures are unrelated to the change made in this PR:

error: unrecognized platform-specific intrinsic function: x86_rdrand16_step
--> /cargo/registry/src/github.com-1ecc6299db9ec823/coresimd-0.1.2/src/coresimd/x86/rdrand.rs:6:5
|
6 | fn x86_rdrand16_step() -> (u16, i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

(Etc.)

@hsivonen
Copy link
Member Author

I'm guessing that the compiler change that broke the simd crate also broke the crates.io-published coresimd crate.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 23, 2019

See #203 (comment)

@BurntSushi
Copy link
Member

I'm not sure I agree with a change like this. It explicitly subverts Rust's stability system, and I would prefer not to see it gain traction in the ecosystem.

@glandium Is there another way to make this work that doesn't involve changing the crate itself?

@SimonSapin
Copy link

The Firefox build system previously had problems with setting RUSTC_BOOTSTRAP=1 on the build system level

Can you say more about those problems? That Firefox abuses this flag is already not good IMO, but making this hack part of a (popular!) crate on crates.io feel like another significant step toward breaking the language’s stability story for everyone.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 28, 2019

Iff we were to add this hack to packed_simd, and Firefox were to use it with a stable compiler, wouldn't Firefox need to cherry pick the commit of packed_simd that built correctly with the nightly that was promoted to that stable compiler version? (I'd guess we could try to do packed_simd releases on crates.io that are sync'ed with those nightlies, but currently we don't do that).

@hsivonen
Copy link
Member Author

Iff we were to add this hack to packed_simd, and Firefox were to use it with a stable compiler, wouldn't Firefox need to cherry pick the commit of packed_simd that built correctly with the nightly that was promoted to that stable compiler version? (I'd guess we could try to do packed_simd releases on crates.io that are sync'ed with those nightlies, but currently we don't do that).

Depends on what kind of compatibility across compiler releases packed_simd will have. So far, the simd crate was practically compatible for years of compiler releases. The latest crates.io release of packed_simd seems to be compatible across multiple compiler releases.

Can you say more about those problems? That Firefox abuses this flag is already not good IMO, but making this hack part of a (popular!) crate on crates.io feel like another significant step toward breaking the language’s stability story for everyone.

@glandium can probably give you a more comprehensive response, but the most obvious thing is that the build.rs solution has per-crate granularity, so compared to setting the environment variable build system-wide, it prevents other crates from developing accidental dependencies on nightly-only features, so this is about preventing uncontrolled proliferation of nightly-only features in crates that Firefox uses.

@Manishearth
Copy link
Member

Can we at least make it so that the build script only sets this flag if a different flag is set? That way you can set that flag globally but crate usere aren't misled into thinking this is stable.

@hsivonen
Copy link
Member Author

Can we at least make it so that the build script only sets this flag if a different flag is set? That way you can set that flag globally but crate usere aren't misled into thinking this is stable.

Works for me. That's how encoding_rs itself does it.

@BurntSushi
Copy link
Member

I still disagree with doing this, even if it's behind an additional flag. It sets a dangerous precedent in the ecosystem. If Firefox needs this for whatever reason, then I'd rather see this solved on the Firefox side rather than establish this as an idiom in the ecosystem.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 28, 2019

FWIW there is an RFC open for stabilizing most of this. The RFC could use an update with the latest features here, and AFAIK the only couple of things blocking it are:

  • ways to mark impl blocks as #[unstable] in the Rust standard library,

There are two optional things that we also wanted to do:

  • (optional) investigating the impact on link times / compile-times of including this module in libcore again (when we removed it, compile-times of some crates that were not using it were reduced by ~10%..). A 10% compile-time regression is probably a blocker, but maybe that fixed itself.

  • (optional) make the shuffle macros proc macros in rustc. Doing this is not a blocker though.

So if using these on stable Rust is a priority / wish for Firefox, they should let the Rust core, library, and compiler teams know so that they can prioritize stabilizing this.

95% of the work is done, but finishing this has extremely low priority in general, and for me as well, since this work has no champion in the libs team, so there is no way for me to push it to completion. Basically this was on hold because Rust2018 had higher priority, and now that Rust2018 shipped it is still on hold because we haven't prioritized what to focus on in 2019. Without a minimum level of commitment from at least one T-libs member, this will never ship.

@hsivonen
Copy link
Member Author

If Firefox needs this for whatever reason, then I'd rather see this solved on the Firefox side rather than establish this as an idiom in the ecosystem.

I considered doing this in a fork, but I though that forking without offering the patch to the upstream first would have been impolite.

So if using these on stable Rust is a priority / wish for Firefox, they should let the Rust core, library, and compiler teams know so that they can prioritize stabilizing this.

I have raised the issue in every Oxidation meeting in every Mozilla All Hands (except the latest one that I didn't attend) since the London meeting in the summer of 2016. I've also included it in both my blog posts in response to both the Rust 2018 and Rust 2019 calls for blog posts.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 31, 2019

I'm sorry to close this henry because I can't really solve this problem here for you. The cargo PR shows that this is a complex topic.

If you want to keep a fork of packed_simd with this PR, that is kind of up to you, I understand. That might allow you to control that the packed_simd version you use compiles and works with the stable compiler that Firefox uses until a better solution to these issues is found. I don't think we will add a build.rs to packed_simd anytime soon, and that part of the Cargo.toml won't change often either, so I don't think you'll get many conflicts while maintaining such a fork.

As I mentioned in the cargo issue, I don't think this is a good solution to this problem, and I'll prefer if this wouldn't need to be done, but I don't fully understand all the constraints that Firefox has.


About stabilizing this, thank you for pushing. I hope we will get this done in 2019. It's been a long time coming. Somebody reported that Rust is now in first place in the nbody benchmark in the benchmarks game, but that the packed_simd implementation of the benchmark in this repo beats that one by a large margin: https://www.reddit.com/r/rust/comments/akgmef/rust_nbody_benchmark_ranks_1/effftr2

That is, the best version in the benchmarks game takes 3.4 seconds on their machine, and the one using packed_simd takes 2.2 seconds. That's a very significant improvement that we should try to land to stable Rust.

@gnzlbg gnzlbg closed this Jan 31, 2019
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.

5 participants