Fix recursion limits with next solver - #9953
Conversation
e1928b5 to
d7c5a2c
Compare
This comment was marked as resolved.
This comment was marked as resolved.
d7c5a2c to
38dc8c9
Compare
38dc8c9 to
2933700
Compare
2933700 to
caf0552
Compare
caf0552 to
ab22123
Compare
3207e16 to
acc16f5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
ErichDonGubler
left a comment
There was a problem hiding this comment.
This is pretty self-contained (minus a licensing issue I noticed), so I see no objection with it.
@jimblandy, @kpreid: I'm going to merge this in the next day or so (pending conversation in fee1-dead/non_structural_derive#1 (comment)), unless you have an objection.
| /// This improves compilation performance and avoids a risk of dependents running into the default | ||
| /// [`recursion_limit`] when checking types containing [`Global`]. This risk will become greater | ||
| /// when Rust’s “next solver” is stabilized. | ||
| /// |
There was a problem hiding this comment.
question(non-blocking): Is there anything we can do to quantitatively substantiate this? Removing the recusion_limit attributes is enough to justify this, OFC, I was just curious if this significantly affected build timings at all (which is how I'd interpret "compilation performance" here).
| /// | ||
| /// [`recursion_limit`]: https://doc.rust-lang.org/reference/attributes/limits.html#the-recursion_limit-attribute | ||
| #[cfg(send_sync)] | ||
| mod global_send_sync { |
There was a problem hiding this comment.
praise: Oooh, I really like keeping this all contained in a module with a single cfg. 😀
| // `-Znext-solver` requires deeper recursion limits (at least for now) to prove Send/Sync | ||
| #![recursion_limit = "256"] |
There was a problem hiding this comment.
praise: It's always nice to avoid recursion_limit attributes. I know that this is the same topic that introduced them in #9609, it's just nice to bend the trade-off curve.
acc16f5 to
1c380d1
Compare
|
Rebased strictly to resolve the conflict that was present in the |
|
Today I was doing some work with nightly, and surprised to find that this PR hasn’t yet been merged. I tried to rebase it to help it along, but I find that I do not know how it should interact with the recent wgpu-core-remote work, most particularly, #10074 saying "Global does not need to be Send or Sync". |
|
I guess the question is, if |
|
From discussion in today’s meeting:
|
|
Should I rebase and re-target this on v30 branch or open a new PR? For the new version, I'm not sure it is worth investing into fixing something that doesn't happen with the default limit, but I can look into that once v30 is fixed if you want. Should be fairly easy now that we know how to address it. |
I think re-targeting this PR makes sense.
Two considerations:
I’d like to try doing it myself. I have an idea for how it should work: instead of trying to find the right place in the internals, how about we do it inside of |
Posted #10177 — a draft PR, to see if CI immediately turns up any problems with this approach. |
755831b to
17d1182
Compare
|
Same 3 commits as before, rebased on |
kpreid
left a comment
There was a problem hiding this comment.
If it were my project, I’d hit the merge (squash) button on this now, but I’ve contributed code to this PR, so I shouldn’t be the one to merge it.
This is missing the additional conclusion that I explicitly drove in the meeting that we were going to opt out of guaranteeing EDIT: Raised: #10177 (review) EDIT 2: Filed a PR for |
|
CC @cwfitzgerald: ☝🏻 This is a backport now. |
…ly (bevyengine#25512) # Objective - Improve compile times in `bevy_render` - Help with bevyengine#25511 ## Solution While investigating a wgpu trait overflow issue with the new solver I noticed that eagerly computing its `Send`/`Sync` reduces the compile time of `bevy_render` by around 10 seconds (on my machine debug goes from 20s to 9s and release from 26s to 15s). I don't like too much having to expose all these types though, and we might get the same perf benefits if gfx-rs/wgpu#9953 is merged though (edit: I tested with that PR changes and that alone reduces compile times by ~6 seconds, however when this PR is added it reduces another ~4 seconds). I also ended up removing the `Arc`s used in some of the wgpu wrapper types, as all wgpu types now use `Arc`s under the hood or equivalent. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
…ly (bevyengine#25512) - Improve compile times in `bevy_render` - Help with bevyengine#25511 While investigating a wgpu trait overflow issue with the new solver I noticed that eagerly computing its `Send`/`Sync` reduces the compile time of `bevy_render` by around 10 seconds (on my machine debug goes from 20s to 9s and release from 26s to 15s). I don't like too much having to expose all these types though, and we might get the same perf benefits if gfx-rs/wgpu#9953 is merged though (edit: I tested with that PR changes and that alone reduces compile times by ~6 seconds, however when this PR is added it reduces another ~4 seconds). I also ended up removing the `Arc`s used in some of the wgpu wrapper types, as all wgpu types now use `Arc`s under the hood or equivalent. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Connections
#9608, #9609
Description
This is a much better alternative to #9609 that solves the same underlying issue. The root cause is that next solver has a really hard time figuring out auto traits and ultimately fails with default recursion limit.
I later discovered that this is infectious, meaning not only
wgpuneeds to raise limits, but any crate that uses a lot of its types. The solution is to help solver a little bit and implement the traits manually. The code is more or less a macro expansion from https://crates.io/crates/non_structural_derive applied to the core type that is used basically everywhere.This small change removes the need to increase recursion limits both here and for downstream users and I believe should help with compilation time as well.
Testing
Checked whether workspace compiles with
cargo clippy.Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.