-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Disable polymorphisation #74633
Disable polymorphisation #74633
Conversation
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.
Can you add the following as a test?
fn test<T>() { std::mem::size_of::<T>(); }
pub fn foo<T>(_: T) -> &'static fn() {
&(test::<T> as fn())
}
fn outer<T>() {
foo(|| ());
}
fn main() {
outer::<u8>();
}
r=me with the test added. We shouldn't roll this up since it will swing perf results again. @bors rollup=never |
This commit changes the span and content of the "collection encountered polymorphic constant" bug in monomorphization collection to point to the use of the constant rather than the definition. Signed-off-by: David Wood <david@davidtw.co>
This commit disables polymorphisation to resolve regressions related to closures which inherit unused generic parameters and are then used in casts or reflection. Signed-off-by: David Wood <david@davidtw.co>
This commit adds a regression test for rust-lang#74614 so that it is fixed before polymorphisation is re-enabled. Signed-off-by: David Wood <david@davidtw.co>
dd9de36
to
799d52e
Compare
@bors r+ p=1 Raising the priority because of nightly breakage |
📌 Commit 799d52e has been approved by |
⌛ Testing commit 799d52e with merge 16b5082861b7528b4f3d171bd6797961b906fdd5... |
@bors retry yield yielding to rollup |
@bors retry |
@bors p=10 for nightly breakage |
☀️ Test successful - checks-actions, checks-azure |
#69749 was a perf loss, but disabling polymorphization was mostly neutral. @davidtwco: is that expected? |
@davidtwco, @wesleywiser -- could someone follow up on this being a performance-neutral PR? Thanks! |
@njn @Mark-Simulacrum Apologies for the delay in looking into this - this was surprising but I think it makes sense; I did some local benchmarking of the PR which landed polymorphisation (with I believe that the majority of the losses that we didn't win back are from cross-crate metadata/incr-comp changes - |
Thanks! Is that something we could plausibly "fix", or does that seem too hard? I guess we're already encoding them as bitsets. Maybe we can skip recording all-zero bitsets and decode them appropriately as well? It may also be worth switching to a lower minimum bound -- u64 implies 64 generics which I expect is way above the normal amount for most functions? |
I think both of these suggestions make sense, I've implemented them in #75155. |
…optimisations, r=lcnr polymorphization: various improvements This PR includes a handful of polymorphisation-related changes: - @Mark-Simulacrum's suggestions [from this comment](rust-lang#74633 (comment)): - Use a `FiniteBitSet<u32>` over a `FiniteBitSet<u64>` as most functions won't have 64 generic parameters. - Don't encode polymorphisation results in metadata when every parameter is used (in this case, just invoking polymorphisation will probably be quicker). - @lcnr's suggestion [from this comment](rust-lang#74717 (comment)). - Add an debug assertion in `ensure_monomorphic_enough` to make sure that polymorphisation did what we expect. r? @lcnr
Fixes #74614.
This PR disables polymorphisation to fix the regression in #74614 after investigation into the issue makes it clear that the fix won't be trivial.
I'll file an issue shortly to replace #74614 with the findings so far.#74636 has been filed to track the fix of the underlying regression.r? @eddyb