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

Introduce Rvalue::ShallowInitBox #89030

Merged
merged 4 commits into from
Sep 25, 2021
Merged

Introduce Rvalue::ShallowInitBox #89030

merged 4 commits into from
Sep 25, 2021

Conversation

nbdd0121
Copy link
Contributor

@nbdd0121 nbdd0121 commented Sep 16, 2021

Polished version of #88700.

Implements MCP rust-lang/compiler-team#460, and should allow #43596 to go forward.

In short, creating an empty box is split from a nullary-op NullOp::Box into two steps, first a call to exchange_malloc, then a Rvalue::ShallowInitBox which transmutes *mut u8 to a shallow-initialized Box<T>. This allows the exchange_malloc call to unwind. Details can be found in the MCP.

NullOp::Box is not yet removed, purely to make reverting easier in case anything goes wrong as the result of this PR. If revert is needed a reversion of "Use Rvalue::ShallowInitBox for box expression" commit followed by a test bless should be sufficient.

Experiments in #88700 showed a very slight compile-time perf regression due to (supposedly) slightly more time spent in LLVM. We could omit unwind edge generation (in non-oom=panic case) in box expression MIR construction to restore perf; but I don't think it's necessary since runtime perf isn't affected and perf difference is rather small.

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 16, 2021
@rust-log-analyzer

This comment has been minimized.

@nbdd0121
Copy link
Contributor Author

The error is due to neglected no_global_oom_handling cfg-gating. I filed #89036 to fix it but that means this PR will be blocked until #89036 is merged :(

the8472 added a commit to the8472/rust that referenced this pull request Sep 22, 2021
Fix missing `no_global_oom_handling` cfg-gating

Cfg-gate these trait impls that are neglected.

These functions compile now because they use `box` syntax which depends on `exchange_malloc` during codegen only; as a result they compiles with cfg `no_global_oom_handling` but shouldn't.

Discovered in rust-lang#89030 because that PR makes `box` syntax depend on `exchange_malloc` lang item during MIR construction.
@michaelwoerister
Copy link
Member

r? @jonas-schievink, who seconded the MCP. I don't know enough about MIR to review this PR.

the8472 added a commit to the8472/rust that referenced this pull request Sep 22, 2021
Fix missing `no_global_oom_handling` cfg-gating

Cfg-gate these trait impls that are neglected.

These functions compile now because they use `box` syntax which depends on `exchange_malloc` during codegen only; as a result they compiles with cfg `no_global_oom_handling` but shouldn't.

Discovered in rust-lang#89030 because that PR makes `box` syntax depend on `exchange_malloc` lang item during MIR construction.
@rust-log-analyzer

This comment has been minimized.

Copy link
Contributor

@jonas-schievink jonas-schievink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look generally good to me! A few notes:

  • This comment can now be removed
  • Is NullOp::Box still used anywhere or can/should it be removed?

compiler/rustc_middle/src/mir/mod.rs Outdated Show resolved Hide resolved
substs: tcx.mk_substs_trait(ty, &[]),
};

self.prove_trait_ref(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is copied from NullaryOp (can the branches be merged?), but I don't get why this is both doing ensure_place_sized and proving that ty: Sized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. With a bit of archaeology I found that this check was introduced in #53575, and with #87935 the Sized constraint is verified in rustc_typeck so the error path will never be hit. The unsized path definitely could be removed.

@@ -2450,6 +2456,10 @@ impl<'tcx> Debug for Rvalue<'tcx> {
}),
}
}

ShallowInitBox(ref place, ref ty) => {
write!(fmt, "ShallowInitBox({:?}, {:?})", place, ty)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks a bit odd in the MIR dumps (with a value and a type being passed the same way), but it does match how NullaryOp is printed, so I guess this can be improved later on.

compiler/rustc_middle/src/mir/tcx.rs Outdated Show resolved Hide resolved
@nbdd0121
Copy link
Contributor Author

Is NullOp::Box still used anywhere or can/should it be removed?

No, it won't be used anymore. My thought is that if we keep it, it'll be easier to revert if anything goes wrong. Reverting "Use Rvalue::ShallowInitBox for box expression" would be enough to bring back the old code path.

If you feel that it should be removed in this PR, I can do that as well.

@rust-log-analyzer

This comment has been minimized.

@jonas-schievink
Copy link
Contributor

No, it won't be used anymore. My thought is that if we keep it, it'll be easier to revert if anything goes wrong. Reverting "Use Rvalue::ShallowInitBox for box expression" would be enough to bring back the old code path.

Seems reasonable!

@bors r+ rollup=never

(for the perf triage team: there's a slight perf impact expected, which was previously measured in #88700 (comment))

@bors
Copy link
Contributor

bors commented Sep 25, 2021

📌 Commit ab64580 has been approved by jonas-schievink

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2021
@bors bors added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 25, 2021
@nbdd0121
Copy link
Contributor Author

GitHub Actions has encountered an internal error when running your job.

Looks like a bogus failure.

@jonas-schievink
Copy link
Contributor

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 25, 2021
@bors
Copy link
Contributor

bors commented Sep 25, 2021

⌛ Testing commit ab64580 with merge e9f29a8...

@bors
Copy link
Contributor

bors commented Sep 25, 2021

☀️ Test successful - checks-actions
Approved by: jonas-schievink
Pushing e9f29a8 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2021
@bors bors merged commit e9f29a8 into rust-lang:master Sep 25, 2021
@rustbot rustbot added this to the 1.57.0 milestone Sep 25, 2021
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e9f29a8): comparison url.

Summary: This change led to large relevant regressions 😿 in compiler performance.

  • Large regression in instruction counts (up to 1.9% on incr-patched: println builds of syn)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Sep 25, 2021
@nbdd0121 nbdd0121 deleted the box2 branch September 26, 2021 00:27
@rylev
Copy link
Member

rylev commented Sep 28, 2021

@nbdd0121 @jonas-schievink I see there was a bit of a justification for the perf regression given when opening this PR:

Experiments in #88700 showed a very slight compile-time perf regression due to (supposedly) slightly more time spent in LLVM. We could omit unwind edge generation (in non-oom=panic case) in box expression MIR construction to restore perf; but I don't think it's necessary since runtime perf isn't affected and perf difference is rather small.

The perf regressions aren't huge but I wouldn't classify themselves as small either. We have some non-noisy, real world crate test cases that are showing regressions that are 9 times larger than the calculated significance threshold. This isn't nearly big enough to merit a revert, but if there's a clear path forward for trying to gain the performance back it's probably worth trying to do that sooner rather than later (considering this is for a feature flagged feature and doesn't really benefit any users at this point). Thoughts?

@nbdd0121
Copy link
Contributor Author

The wall-clock time of some of these "regressions" are improved though; instruction count isn't the full picture.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 28, 2021
Introduce `Rvalue::ShallowInitBox`

Polished version of rust-lang#88700.

Implements MCP rust-lang/compiler-team#460, and should allow rust-lang#43596 to go forward.

In short, creating an empty box is split from a nullary-op `NullOp::Box` into two steps, first a call to `exchange_malloc`, then a `Rvalue::ShallowInitBox` which transmutes `*mut u8` to a shallow-initialized `Box<T>`. This allows the `exchange_malloc` call to unwind. Details can be found in the MCP.

`NullOp::Box` is not yet removed, purely to make reverting easier in case anything goes wrong as the result of this PR. If revert is needed a reversion of "Use Rvalue::ShallowInitBox for box expression" commit followed by a test bless should be sufficient.

Experiments in rust-lang#88700 showed a very slight compile-time perf regression due to (supposedly) slightly more time spent in LLVM. We could omit unwind edge generation (in non-`oom=panic` case) in box expression MIR construction to restore perf; but I don't think it's necessary since runtime perf isn't affected and perf difference is rather small.
@rylev
Copy link
Member

rylev commented Sep 28, 2021

@nbdd0121 wall-clock time is an extremely noisy metric which we don't usually look at unless the gains are consistent and large which is not the case here. Of course, instruction count is only part of the picture, but it's generally the best we have for determining runtime performance changes, so I still think discussing the targeted fixed is warranted.

@nbdd0121 nbdd0121 mentioned this pull request Oct 6, 2021
@nbdd0121 nbdd0121 mentioned this pull request Oct 20, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 3, 2022
Remove `NullOp::Box`

Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460.

~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely.

r? `@jonas-schievink`
`@rustbot` label T-compiler
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jan 13, 2022
Remove `NullOp::Box`

Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460.

~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely.

r? `@jonas-schievink`
`@rustbot` label T-compiler
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Feb 23, 2022
Remove `NullOp::Box`

Follow up of rust-lang#89030 and MCP rust-lang/compiler-team#460.

~1 month later nothing seems to be broken, apart from a small regression that rust-lang#89332 (1aac85bb716c09304b313d69d30d74fe7e8e1a8e) shows could be regained by remvoing the diverging path, so it shall be safe to continue and remove `NullOp::Box` completely.

r? `@jonas-schievink`
`@rustbot` label T-compiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants