Remove box syntax from compiler and tools #87781
Conversation
|
r? @oli-obk (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
@bors try |
|
⌛ Trying commit 0e42280 with merge 48ad90101547f3344a58a700a4dbcb4590a14e2b... |
|
#49733 says
That sounds to me like it's not at all certain that it's going to be removed. |
|
@jyn514 good point but note that the issue also contains:
This PR alleviates the "widespread usage in the compiler" point, if wanted. Also note that currently the compiler has inconsistent usage of |
|
@rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
(also I have edited the PR description to remove the claim that the tracking issue plans certain removal) |
|
☀️ Try build successful - checks-actions |
|
Queued 48ad90101547f3344a58a700a4dbcb4590a14e2b with parent d4ad1cf, future comparison URL. |
|
One question though: Without the |
|
Finished benchmarking try commit (48ad90101547f3344a58a700a4dbcb4590a14e2b): comparison url. Summary: This benchmark run did not return any significant changes. If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. @bors rollup=never |
rustbot
removed
the
S-waiting-on-perf
Another benchmark worth doing is to turn all of them into box syntax. |
It could call the allocator without lang item indirection like it currently does ( |
|
If I see it correctly, you haven't removed every occurence of the I really would like to see that. I think it would be easiest to remove the features Getting rid of |
est31
changed the title
|
Please do not remove box_patterns. It makes code vastly more easy to read and it also has a potential replacement with the Deref pattern project. |
|
Can we have another timer run? |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
|
I don't think a straight revert is necessarily the right approach -- the regressions here are tiny, realistically, and wall times do not show a corresponding significant decrease. Cachegrind diff for serde-doc point to a particular set of functions which likely drive the regressions -- targeted reverts likely make more sense here than a blanket revert. That said, if we revert everything in rustdoc for now that doesn't seem the end of the world either. Ultimately, this just points to Box::new being slightly less able to optimize than box, which was already well known. |
|
I also see no particular reason to believe that collect_blanket_impls has been affected by this patch -- can you say more about that perhaps? |
|
I've filed a PR in #89134 reverting every change of librustdoc. If anything can stay, please point it out! |
…GuillaumeGomez Revert the rustdoc box syntax removal Reverts the rustdoc box syntax removal from rust-lang#87781. It turned out to cause (minor) perf regressions. Requested in rust-lang#87781 (comment)
…illaumeGomez Revert the rustdoc box syntax removal Reverts the rustdoc box syntax removal from rust-lang#87781. It turned out to cause (minor) perf regressions. Requested in rust-lang#87781 (comment)
…crum Remove most box syntax uses from the testsuite except for src/test/ui/issues Removes most box syntax uses from the testsuite outside of the src/test/ui/issues directory. The goal was to only change tests where box syntax is an implementation detail instead of the actual feature being tested. So some tests were left out, like the regression test for rust-lang#87935, or tests where the obtained error message changed significantly. Mostly this replaces box syntax with `Box::new`, but there are some minor drive by improvements, like formatting improvements or `assert_eq` instead of `assert!( == )`. Prior PR that removed box syntax from the compiler and tools: rust-lang#87781
????? try {
a = new some_constructor(some_args);
}catch(...){
//...
}Here we can check for allocation errors while directly constructing value in place! |
|
On nightly you can use #![feature(new_uninit)]
let mut five = Box::<u32>::new_uninit();
let five: Box<u32> = unsafe {
// Deferred initialization:
five.as_mut_ptr().write(5);
five.assume_init()
};You can do something similar for the |
Remove box syntax from rustc_mir_dataflow and rustc_mir_transform Continuation of rust-lang#87781, inspired by rust-lang#97239. The usages that this PR removes have not appeared from nothing, instead the usage in `rustc_mir_dataflow` and `rustc_mir_transform` was from rust-lang#80522 which split up `rustc_mir`, and which was filed before I filed rust-lang#87781, so it was using the state from before my PR. But it was merged after my PR was merged, so the `box_syntax` uses were able to survive here. Outside of this introduction due to the code being outside of the master branch at the point of merging of my PR, there was only one other introduction of box syntax, in rust-lang#95159. That box syntax was removed again though in rust-lang#95555. Outside of that, `box_syntax` has not made its reoccurrance in compiler crates.
Add #[rustc_box] and use it inside alloc
This commit adds an alternative content boxing syntax, and uses it inside alloc.
```Rust
#![feature(box_syntax)]
fn foo() {
let foo = box bar;
}
```
is equivalent to
```Rust
#![feature(rustc_attrs)]
fn foo() {
let foo = #[rustc_box] Box::new(bar);
}
```
The usage inside the very performance relevant code in
liballoc is the only remaining relevant usage of box syntax
in the compiler (outside of tests, which are comparatively easy to port).
box syntax was originally designed to be used by all Rust
developers. This introduces a replacement syntax more tailored
to only being used inside the Rust compiler, and with it,
lays the groundwork for eventually removing box syntax.
[Earlier work](rust-lang#87781 (comment)) by `@nbdd0121` to lower `Box::new` to `box` during THIR -> MIR building ran into borrow checker problems, requiring the lowering to be adjusted in a way that led to [performance regressions](rust-lang#87781 (comment)). The proposed change in this PR lowers `#[rustc_box] Box::new` -> `box` in the AST -> HIR lowering step, which is way earlier in the compiler, and thus should cause less issues both performance wise as well as regarding type inference/borrow checking/etc. Hopefully, future work can move the lowering further back in the compiler, as long as there are no performance regressions.
Remove most box syntax from librustdoc This is the second attempt after the librustdoc specific changes have been reverted from rust-lang#87781 in rust-lang#89134, due to a minor, but exant regression caused by the changes. ~~There have been some changes to librustdoc in the past and maybe thanks to them there is no regression any more. If there is still a regression, one can investigate further and maybe find ways to fix the regressions. Thus, i request a perf run.~~ Edit: turns out there is still a regression, but it's caused only by a subset of the changes. So I've changed this PR to only contains the changes that don't cause any performance regressions, keeping the regression causing changes for a later PR.
Add #[rustc_box] and use it inside alloc
This commit adds an alternative content boxing syntax, and uses it inside alloc.
```Rust
#![feature(box_syntax)]
fn foo() {
let foo = box bar;
}
```
is equivalent to
```Rust
#![feature(rustc_attrs)]
fn foo() {
let foo = #[rustc_box] Box::new(bar);
}
```
The usage inside the very performance relevant code in
liballoc is the only remaining relevant usage of box syntax
in the compiler (outside of tests, which are comparatively easy to port).
box syntax was originally designed to be used by all Rust
developers. This introduces a replacement syntax more tailored
to only being used inside the Rust compiler, and with it,
lays the groundwork for eventually removing box syntax.
[Earlier work](rust-lang/rust#87781 (comment)) by `@nbdd0121` to lower `Box::new` to `box` during THIR -> MIR building ran into borrow checker problems, requiring the lowering to be adjusted in a way that led to [performance regressions](rust-lang/rust#87781 (comment)). The proposed change in this PR lowers `#[rustc_box] Box::new` -> `box` in the AST -> HIR lowering step, which is way earlier in the compiler, and thus should cause less issues both performance wise as well as regarding type inference/borrow checking/etc. Hopefully, future work can move the lowering further back in the compiler, as long as there are no performance regressions.
Removes box syntax from the compiler and tools. In #49733, the future of box syntax is uncertain and the use in the compiler was listed as one of the reasons to keep it. Removal of box syntax might affect the code generated and slow down the compiler so I'd recommend doing a perf run on this.