Skip to content

polymorphization: various improvements #75155

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 5, 2020
Merged

polymorphization: various improvements #75155

merged 3 commits into from
Aug 5, 2020

Conversation

davidtwco
Copy link
Member

This PR includes a handful of polymorphisation-related changes:

  • @Mark-Simulacrum's suggestions from this 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.
    • Add an debug assertion in ensure_monomorphic_enough to make sure that polymorphisation did what we expect.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 4, 2020
This was referenced Aug 4, 2020
Merged
Merged
lcnr
lcnr reviewed Aug 4, 2020
View reviewed changes
@davidtwco
mir: use FiniteBitSet<u32> in polymorphization
5f89f02 
This commit changes polymorphization to return a `FiniteBitSet<u32>`
rather than a `FiniteBitSet<u64>` because most functions do not use
anywhere near sixty-four generic parameters so keeping a `u64` around is
unnecessary in most cases.

Signed-off-by: David Wood <[email protected]>
@davidtwco davidtwco force-pushed the polymorphization-incr-comp-optimisations branch from aae7ad4 to e040736 Compare August 4, 2020 17:31
@rust-lang rust-lang deleted a comment from davidtwco Aug 4, 2020
lcnr
lcnr approved these changes Aug 4, 2020
View reviewed changes
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.

r=me

Do you want to deal with my nits here?

@lcnr
Copy link
Contributor

lcnr commented Aug 4, 2020

Also @bors rollup=never

@davidtwco
Copy link
Member Author

Do you want to deal with my nits here?

I'll address those here but I won't get around to it until tomorrow (turned everything off for the day).

@davidtwco davidtwco force-pushed the polymorphization-incr-comp-optimisations branch from e040736 to adea212 Compare August 4, 2020 19:07
@davidtwco
Copy link
Member Author

Fixed those nits, it was bugging me knowing that the PR had unresolved comments.

@davidtwco
metadata: skip empty polymorphization bitset
70b49c7 
This commit skips encoding empty polymorphization results - while
polymorphization is disabled, this should be every polymorphization
result; but when polymorphization is re-enabled, this would help with
non-generic functions and those which do use all their parameters (most
functions).

Signed-off-by: David Wood <[email protected]>
@davidtwco
mir: add debug assertion to check polymorphization
63fadee 
This commit adds some debug assertions to `ensure_monomorphic_enough`
which checks that unused generic parameters have been replaced with a
parameter.

Signed-off-by: David Wood <[email protected]>
@davidtwco davidtwco force-pushed the polymorphization-incr-comp-optimisations branch from adea212 to 63fadee Compare August 4, 2020 19:09
@lcnr
Copy link
Contributor

lcnr commented Aug 4, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Aug 4, 2020

⌛ Trying commit 63fadee with merge 7a4d90c2e80578e57956455e81b511d8fb964d70...

@bors
Copy link
Collaborator

bors commented Aug 4, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 7a4d90c2e80578e57956455e81b511d8fb964d70 (7a4d90c2e80578e57956455e81b511d8fb964d70)

@rust-timer
Copy link
Collaborator

Queued 7a4d90c2e80578e57956455e81b511d8fb964d70 with parent 5f6bd6e, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (7a4d90c2e80578e57956455e81b511d8fb964d70): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@lcnr
Copy link
Contributor

lcnr commented Aug 5, 2020

Nice

@bors r+ rollup=never

@bors
Copy link
Collaborator

bors commented Aug 5, 2020

📌 Commit 63fadee has been approved by lcnr

@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 Aug 5, 2020
@bors
Copy link
Collaborator

bors commented Aug 5, 2020

⌛ Testing commit 63fadee with merge 7f8ff84...

@bors
Copy link
Collaborator

bors commented Aug 5, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: lcnr
Pushing 7f8ff84 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 5, 2020
@bors bors merged commit 7f8ff84 into rust-lang:master Aug 5, 2020
@davidtwco davidtwco deleted the polymorphization-incr-comp-optimisations branch August 5, 2020 09:16
@panstromek
Copy link
Contributor

panstromek commented Aug 5, 2020 •
edited
Loading

I don't understand how can this be a perf improvement when the polymorphization is still disabled. Does the pass run even when disabled but results are just not used?

Use a FiniteBitSet over a FiniteBitSet as most functions won't have 64 generic parameters.

Well, most functions won't have 32 generic parameters either 😄, I am curious if u16 or even u8 could be even better improvement?

@davidtwco
Copy link
Member Author

I don't understand how can this be a perf improvement when the polymorphization is still disabled. Does the pass run even when disabled but results are not used?

Polymorphisation still gets invoked but immediately returns with a result indicating that all generic parameters are used.

Other parts of the PR which introduced polymorphisation are still present. Among these changes, the results of polymorphisation are being written to the cross-crate metadata so when polymorphisation is invoked while disabled, it would still preferentially load that from the cross-crate metadata - that's slower when the query is really fast (e.g. when polymorphisation is disabled).

See this comment for this phrased differently.

@panstromek
Copy link
Contributor

I see, that makes sense. Thanks for clarification 😉

mrkline added a commit to mrkline/backpak that referenced this pull request Jan 17, 2024
@mrkline
b2: Don't base code on old blog posts
cef1dc8 
I tried an inner function from a vague recollection of
https://matklad.github.io/2023/01/26/rusts-ugly-syntax.html

But it turns out compilers are good at compiler things:
rust-lang/rust#75155
@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
Reviewers

@lcnr lcnr lcnr approved these changes

Assignees

@lcnr lcnr

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
Milestone
1.47.0
Development

Successfully merging this pull request may close these issues.
7 participants
@davidtwco @lcnr @rust-timer @bors @panstromek @cuviper @rust-highfive