Add opt-for-size core lib feature flag #125011
Conversation
rustbot
added
S-waiting-on-review
|
You may also need to add it to |
Oh right, the abort feature is there too. I've got no idea really what sysroot is, but I've added it there too |
scottmcm
added
the
I-libs-nominated
|
This sounds like a team question, not a reviewer question. |
|
No objection to the concept, but I'd be very, very hesitant to add a large number of alternative algorithm implementations. (And if we ever want to support this option in any stable way, I think we'd need to run the testsuite on a version compiled that way, to make sure that the alternatives work.) |
|
We discussed this in the library team meeting today and we're happy to add this flag. However one concern was raised: we would like the alternative implementations of algorithms behind this flag to actually be tested in CI. The easiest way is probably to re-run the standard library tests with this feature enabled in the CI scripts. Or if this is too expensive then it could be done only for a single target. |
Amanieu
removed
the
I-libs-nominated
Should the test infra be added in this PR, a followup PR or together with the first PR that actually uses this flag? |
|
I can help with setting up the CI, let me know if you want to add it to this PR. |
@Kobzol yeah let's do it! I've been looking for something to do the next hackday (this friday) at work. (And if it takes longer than a day, that's fine too) Looking at the CI scripts I feel kinda lost. So if you could write down and link to where I should be looking, that'd be much appreciated. |
|
I digged into stdlib tests a bit and realized that it's not so simple. I'm not actually sure if we already run tests for panic="abort" for stdlib. I found a way to do this, but maybe it's a bit hacky. I'll lead the discussion here. |
|
Additionally it would be useful to track the size of a standard library built this way. This could happen in rustc-perf or as some script that prints out the size which can be used for before/after comparisons when someone lands additional implementations under this cfg. But neither has to happen in this PR. |
|
Tracking the size would be nice indeed, but also probably hard to do. Afaik you can't really measure the size of a library very well |
rustbot
added
A-testsuite
|
Added the flag to the CI. I think this only runs on merges. |
|
Thanks! @bors r=Amanieu,kobzol We don't currently have an easy way of monitoring the size of stdlib with this flag enabled, but for now it will be probably enough to just post a size diff summary on PRs that do something with this flag. |
bors
added
S-waiting-on-bors
|
Should we have a tracking issue for the effort? Or some documentation? |
|
I'm not sure if it should be a tracking issue, as there is not really any specific end goal here to track. The expected usage is that eventually, people will start sending targeted PRs to reduce specific code bloat in the stdlib when the flag is enabled (e.g. using a sort algorithm optimized for small binary size). That being said, we should definitely document this somehow. Maybe here? This is also relevant for |
Right now the feature isn't useful, so I'd say a minimum goal would be getting it up to some shape where it has tangible benefits so that nightly users would want to opt into it. |
|
Once this lands in nightly I want to make some prs that are easy wins based on what I experimented before. |
…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#124570 (Miscellaneous cleanups) - rust-lang#124772 (Refactor documentation for Apple targets) - rust-lang#125011 (Add opt-for-size core lib feature flag) - rust-lang#125218 (Migrate `run-make/no-intermediate-extras` to new `rmake.rs`) - rust-lang#125225 (Use functions from `crt_externs.h` on iOS/tvOS/watchOS/visionOS) - rust-lang#125266 (compiler: add simd_ctpop intrinsic) - rust-lang#125348 (Small fixes to `std::path::absolute` docs) Failed merges: - rust-lang#125296 (Fix `unexpected_cfgs` lint on std) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#125011 - diondokter:opt-for-size, r=Amanieu,kobzol Add opt-for-size core lib feature flag Adds a feature flag to the core library that enables the possibility to have smaller implementations for certain algorithms. So far, the core lib has traded performance for binary size. This is likely what most people want since they have big simd-capable machines. However, people on small machines, like embedded devices, don't enjoy the potential speedup of the bigger algorithms, but do have to pay for them. These microcontrollers often only have 16-1024kB of flash memory. This PR is the result of some talks with project members like `@Amanieu` at RustNL. There are some open questions of how this is eventually stabilized, but it's a similar question as with the existing `panic_immediate_abort` feature. Speaking as someone from the embedded side, we'd rather have this unstable for a while as opposed to not having it at all. In the meantime we can try to use it and also add additional PRs to the core lib that uses the feature flag in areas where we find benefit. Open questions from my side: - Is this a good feature name? - `panic_immediate_abort` is fairly verbose, so I went with something equally verbose - It's easy to refactor later - I've added the feature to `std` and `alloc` as well as they might benefit too. Do we agree? - I expect these to get less usage out of the flag since most size-constraint projects don't use these libraries often.
|
Tracking issue for this flag: #125612 |
bootstrap: don't use rustflags for `--rustc-args` r? `@onur-ozkan` This is going to require a bit of context. rust-lang#47558 has added `--rustc-args` to `./x test` to allow passing flags when building `compiletest` tests. It was made specifically because using `RUSTFLAGS` would rebuild the compiler/stdlib, which would in turn require the flag you want to build tests with to successfully bootstrap. rust-lang#113178 made the request that it also works for other tests and doctests. This is not trivial to support across the board for `library`/`compiler` unit-tests/doctests and across stages. This issue was closed in rust-lang#113948 by using `RUSTFLAGS`, seemingly incorrectly since rust-lang#123489 fixed that part to make it work. Unfortunately rust-lang#123489/rust-lang#113948 have regressed the goals of `--rustc-args`: - now we can't use rustc args that don't bootstrap, to run the UI tests: we can't test incomplete features. The new trait solver doesn't bootstrap, in-progress borrowck/polonius changes don't bootstrap, some other features are similarly incomplete, etc. - using the flag now rebuilds everything from scratch: stage0 stdlib, stage1 compiler, stage1 stdlib. You don't need to re-do all this to compile UI tests, you only need the latter to run stdlib tests with a new flag, etc. This happens for contributors, but also on CI today. (Not to mention that in doing that it will rebuild things with flags that are not meant to be used, e.g. stdlib cfgs that don't exist in the compiler; or you could also imagine that this silently enables flags that were not meant to be enabled in this way). Since then, rust-lang@bd71c71 has started using it to test a stdlib feature, relying on the fact that it now rebuilds everything. So rust-lang#125011 also regressed CI times more than necessary because it rebuilds everything instead of just stage 1 stdlib. It's not easy for me to know how to properly fix rust-lang#113178 in bootstrap, but rust-lang#113948/rust-lang#123489 are not it since they regress the initial intent. I'd think bootstrap would have to know from the list of test targets that are passed that the `library` or `compiler` paths that are passed could require rebuilding these crates with different rustflags, probably also depending on stages. Therefore I would not be able to fix it, and will just try in this PR to unregress the situation to unblock the initial use-case. It seems miri now also uses `./x miri --rustc-args` in this incorrect meaning to rebuild the `library` paths they support to run with the new args. I've not made any bootstrap changes related to `./x miri` in this PR, so `--rustc-args` wouldn't work there anymore. I'd assume this would need to use rustflags again but I don't know how to make that work properly in bootstrap, hence opening as draft, so you can tell me how to do that. I assume we don't want to break their use-case again now that it exists, even though there are ways to use `./x test` to do exactly that. `RUSTFLAGS_NOT_BOOTSTRAP=flag ./x test library/std` is a way to run unit tests with a new flag without rebuilding everything, while with rust-lang#123489 there is no way anymore to run tests with a flag that doesn't bootstrap. --- edit: after review, this PR: - renames `./x test --rustc-args` to `./x test --compiletest-rustc-args` as it only applies there, and cannot use rustflags for this purpose. - fixes the regression that using these args rebuilt everything from scratch - speeds up some CI jobs via the above point - removes `./x miri --rustc-args` as only library tests are supported, needs to rebuild libstd, and `./x miri --compiletest-rustc-args` wouldn't work since compiletests are not supported.
Rollup merge of rust-lang#128841 - lqd:rustc-args, r=onur-ozkan bootstrap: don't use rustflags for `--rustc-args` r? `@onur-ozkan` This is going to require a bit of context. rust-lang#47558 has added `--rustc-args` to `./x test` to allow passing flags when building `compiletest` tests. It was made specifically because using `RUSTFLAGS` would rebuild the compiler/stdlib, which would in turn require the flag you want to build tests with to successfully bootstrap. rust-lang#113178 made the request that it also works for other tests and doctests. This is not trivial to support across the board for `library`/`compiler` unit-tests/doctests and across stages. This issue was closed in rust-lang#113948 by using `RUSTFLAGS`, seemingly incorrectly since rust-lang#123489 fixed that part to make it work. Unfortunately rust-lang#123489/rust-lang#113948 have regressed the goals of `--rustc-args`: - now we can't use rustc args that don't bootstrap, to run the UI tests: we can't test incomplete features. The new trait solver doesn't bootstrap, in-progress borrowck/polonius changes don't bootstrap, some other features are similarly incomplete, etc. - using the flag now rebuilds everything from scratch: stage0 stdlib, stage1 compiler, stage1 stdlib. You don't need to re-do all this to compile UI tests, you only need the latter to run stdlib tests with a new flag, etc. This happens for contributors, but also on CI today. (Not to mention that in doing that it will rebuild things with flags that are not meant to be used, e.g. stdlib cfgs that don't exist in the compiler; or you could also imagine that this silently enables flags that were not meant to be enabled in this way). Since then, rust-lang@bd71c71 has started using it to test a stdlib feature, relying on the fact that it now rebuilds everything. So rust-lang#125011 also regressed CI times more than necessary because it rebuilds everything instead of just stage 1 stdlib. It's not easy for me to know how to properly fix rust-lang#113178 in bootstrap, but rust-lang#113948/rust-lang#123489 are not it since they regress the initial intent. I'd think bootstrap would have to know from the list of test targets that are passed that the `library` or `compiler` paths that are passed could require rebuilding these crates with different rustflags, probably also depending on stages. Therefore I would not be able to fix it, and will just try in this PR to unregress the situation to unblock the initial use-case. It seems miri now also uses `./x miri --rustc-args` in this incorrect meaning to rebuild the `library` paths they support to run with the new args. I've not made any bootstrap changes related to `./x miri` in this PR, so `--rustc-args` wouldn't work there anymore. I'd assume this would need to use rustflags again but I don't know how to make that work properly in bootstrap, hence opening as draft, so you can tell me how to do that. I assume we don't want to break their use-case again now that it exists, even though there are ways to use `./x test` to do exactly that. `RUSTFLAGS_NOT_BOOTSTRAP=flag ./x test library/std` is a way to run unit tests with a new flag without rebuilding everything, while with rust-lang#123489 there is no way anymore to run tests with a flag that doesn't bootstrap. --- edit: after review, this PR: - renames `./x test --rustc-args` to `./x test --compiletest-rustc-args` as it only applies there, and cannot use rustflags for this purpose. - fixes the regression that using these args rebuilt everything from scratch - speeds up some CI jobs via the above point - removes `./x miri --rustc-args` as only library tests are supported, needs to rebuild libstd, and `./x miri --compiletest-rustc-args` wouldn't work since compiletests are not supported.
Adds a feature flag to the core library that enables the possibility to have smaller implementations for certain algorithms.
So far, the core lib has traded performance for binary size. This is likely what most people want since they have big simd-capable machines. However, people on small machines, like embedded devices, don't enjoy the potential speedup of the bigger algorithms, but do have to pay for them. These microcontrollers often only have 16-1024kB of flash memory.
This PR is the result of some talks with project members like @Amanieu at RustNL.
There are some open questions of how this is eventually stabilized, but it's a similar question as with the existing
panic_immediate_abortfeature.Speaking as someone from the embedded side, we'd rather have this unstable for a while as opposed to not having it at all. In the meantime we can try to use it and also add additional PRs to the core lib that uses the feature flag in areas where we find benefit.
Open questions from my side:
panic_immediate_abortis fairly verbose, so I went with something equally verbosestdandallocas well as they might benefit too. Do we agree?