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

Disable polymorphisation #74633

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Jul 22, 2020

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 22, 2020
Copy link
Contributor

@lcnr lcnr left a 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>();
}

@wesleywiser
Copy link
Member

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>
@wesleywiser
Copy link
Member

@bors r+ p=1

Raising the priority because of nightly breakage

@bors
Copy link
Contributor

bors commented Jul 22, 2020

📌 Commit 799d52e has been approved by wesleywiser

@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 Jul 22, 2020
@bors
Copy link
Contributor

bors commented Jul 22, 2020

⌛ Testing commit 799d52e with merge 16b5082861b7528b4f3d171bd6797961b906fdd5...

@Manishearth
Copy link
Member

@bors retry yield

yielding to rollup

@Aaron1011
Copy link
Member

@bors retry

@lqd
Copy link
Member

lqd commented Jul 22, 2020

@bors p=10 for nightly breakage

@bors
Copy link
Contributor

bors commented Jul 22, 2020

⌛ Testing commit 799d52e with merge bbebe73...

@bors
Copy link
Contributor

bors commented Jul 22, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: wesleywiser
Pushing bbebe73 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 22, 2020
@bors bors merged commit bbebe73 into rust-lang:master Jul 22, 2020
@davidtwco davidtwco deleted the issue-74614-disable-polymorphisation branch July 24, 2020 09:53
@nnethercote
Copy link
Contributor

#69749 was a perf loss, but disabling polymorphization was mostly neutral. @davidtwco: is that expected?

@Mark-Simulacrum
Copy link
Member

@davidtwco, @wesleywiser -- could someone follow up on this being a performance-neutral PR? Thanks!

@davidtwco
Copy link
Member Author

#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 -Zpolymorphisation=off) and the merge before that.

I believe that the majority of the losses that we didn't win back are from cross-crate metadata/incr-comp changes - -Zpolymorphize=off makes unused_generic_params always return every-parameter-is-used, but that result is still serialized - there was nothing to serialize without polymorphisation having landed. Analysing perf results isn't something I've got much experience with, but as far as I can tell, this explains the inconsistency here.

@Mark-Simulacrum
Copy link
Member

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?

@davidtwco
Copy link
Member Author

Thanks! Is that something we could plausibly "fix", or does that seem too hard?

I think both of these suggestions make sense, I've implemented them in #75155.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 5, 2020
…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
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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. 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.

Regression in nightly-2020-07-22: collection encountered polymorphic constant