-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add #[rustc_box] and use it inside alloc #97293
Conversation
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
r? @kennytm (rust-highfive has picked a reviewer for you, use r? to override) |
Oh right and I'd like a perf run for this. @bors try |
⌛ Trying commit 21b8f4c5732fa39970597845a14656dd92d5aa81 with merge c40cfb8bd5f0f25521b77d628fc502e48839db62... |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit b3da9d276364534550d8f71c5b6f1c21d983b2ac with merge 7a4b510ef15bc589ea430b0bd23134b71f4e25d2... |
☀️ Try build successful - checks-actions |
Queued 7a4b510ef15bc589ea430b0bd23134b71f4e25d2 with parent b2eed72, future comparison URL. |
Finished benchmarking commit (7a4b510ef15bc589ea430b0bd23134b71f4e25d2): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
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. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Footnotes |
Hmm so the only serious regression is in Instruction count, with regressions only in Max RSS, a lot of improvements with regressions in one place: So this is IMO quite a good result. |
The weird thing is that these changes are still larger than I expected. After all, the differences are only at the start of the compilation pipeline. So regressions in later phases, which these mostly are, are a bit surprising. One might think that the ucd library has the memory usage regression because of the many tables that it contains. And indeed it has many tables. But there is not a single usage of the For Really bad Not so bad The weird part is that this PR does not change anything what is being fed to LLVM. So why does LLVM take longer? No idea. I think I need to create a local build of the compiler as of this PR and play around with it to verify that the MIR etc. is all the same. Edit: plus, given that it's LLVM, there is also barely any Rust running in that pass so it also can't be some general issue of the rust-written part compiler being slower now, because it uses the (hypothetically) now-slower |
I've got an idea to narrow the performance changes down: what if one only changes the |
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit 704845b4efe169fa881bef052db59d163c74d433 with merge 6f102539baaab210a6d285cc923d5d22e4959ac0... |
Alright, the last commit is removed and pushed to a separate branch for possible future use. The PR is ready for review. |
hir::ExprKind::Box(self.lower_expr(&inner)) | ||
let kind = hir::ExprKind::Box(self.lower_expr(&inner)); | ||
let hir_id = self.lower_node_id(e.id); | ||
self.lower_attrs(hir_id, &e.attrs[1..]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this assumes that rustc_box
is the first attribute? If rustc_box
is assumed to be the only attribute, then we could just skip lowering attributes at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My reasoning for assuming it was first was that rustc_box
shouldn't appear so often and I wouldn't want to do a possibly expensive search over all attributes. Once we have rustc_box
, it should be comparatively inexpensive to take care of the other attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(When making that decision, I thought about doc attributes but function calls obviously don't have doc attributes so I was a bit misguided)
This commit adds an alternative content boxing syntax, and uses it inside alloc. 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.
r? @oli-obk |
I'm surprised it wasn't possible to do on the THIR. Looking at the links you provided it looks like they didn't use a |
@bors r+ |
📌 Commit 0a24b94 has been approved by |
☀️ Test successful - checks-actions |
Finished benchmarking commit (20976ba): comparison url. Instruction count
Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
This commit adds an alternative content boxing syntax, and uses it inside alloc.
is equivalent to
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 by @nbdd0121 to lower
Box::new
tobox
during THIR -> MIR building ran into borrow checker problems, requiring the lowering to be adjusted in a way that led to performance regressions. 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.