Skip to content

Removing the def_id field from hot ParamEnv to make it smaller #76244

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
bors merged 1 commit into from
Sep 13, 2020
Merged

Removing the def_id field from hot ParamEnv to make it smaller #76244

bors merged 1 commit into from
Sep 13, 2020

Conversation

@vandenheuvel
Copy link
Contributor

@vandenheuvel vandenheuvel commented Sep 2, 2020 •
edited
Loading

This PR addresses #74865.

@rust-highfive
Copy link
Contributor

rust-highfive commented Sep 2, 2020

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 2, 2020
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
@vandenheuvel vandenheuvel force-pushed the remove__paramenv__def_id branch from 71cb92b to 6f44404 Compare September 2, 2020 11:59
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
@vandenheuvel vandenheuvel force-pushed the remove__paramenv__def_id branch from 6f44404 to 0d7ab10 Compare September 2, 2020 12:28
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
vandenheuvel
vandenheuvel commented Sep 2, 2020
View reviewed changes
compiler/rustc_typeck/src/outlives/explicit.rs Outdated
Copy link
Contributor Author

@vandenheuvel vandenheuvel Sep 2, 2020

Choose a reason for hiding this comment

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

What would you think about changing this list (which is also a duplicate) to a _ pattern?

@vandenheuvel vandenheuvel force-pushed the remove__paramenv__def_id branch from 0d7ab10 to 047f03f Compare September 2, 2020 13:14
jackh726
jackh726 requested changes Sep 2, 2020
View reviewed changes
@vandenheuvel vandenheuvel force-pushed the remove__paramenv__def_id branch from d88ca38 to db6b482 Compare September 2, 2020 18:03
jackh726
jackh726 reviewed Sep 2, 2020
View reviewed changes
@jackh726
Copy link
Member

jackh726 commented Sep 2, 2020

Also, seems like one of the test outputs changed, so you'll have to ./x.py test --bless

jackh726
jackh726 reviewed Sep 2, 2020
View reviewed changes
nikomatsakis
nikomatsakis approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This looks great! Nice work. I left two nits about naming, but otherwise I'm happy.

compiler/rustc_ty/src/ty.rs Outdated
Copy link
Contributor

@nikomatsakis nikomatsakis Sep 2, 2020

Choose a reason for hiding this comment

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

can we add a doc comment to this fn? it's pretty non-obvious to me what it does from its name. Actually, I suspect the name should just be improved to something like well_formed_types_in_env

Copy link
Contributor Author

@vandenheuvel vandenheuvel Sep 2, 2020

Choose a reason for hiding this comment

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

/// Elaborate the environment.
///
/// Collect a list of `Predicate`'s used for building the `ParamEnv`.
/// 
/// Used only in chalk mode.

@jackh726 anything to add?

Copy link
Member

@jackh726 jackh726 Sep 2, 2020

Choose a reason for hiding this comment

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

Perhaps mention that this adds TypeFromEnv (or what the eventual name ends up being) predicates. Specifically, because these types come from the environment, we assume they are well formed.

@jonas-schievink
Copy link
Contributor

jonas-schievink commented Sep 2, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

rust-timer commented Sep 2, 2020

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Sep 2, 2020

⌛ Trying commit 8f39a0415a6ca789a18c9d3bbbba43b5fda542bd with merge 5ef250dd2ad618ee339f165e9b711a1b4746887d...

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 11, 2020
@panstromek panstromek mentioned this pull request Sep 12, 2020
Closed
@jonas-schievink
Copy link
Contributor

jonas-schievink commented Sep 13, 2020

@bors r=nikomatsakis

@bors
Copy link
Collaborator

bors commented Sep 13, 2020

📌 Commit 7dad29d has been approved by nikomatsakis

@bors
Copy link
Collaborator

bors commented Sep 13, 2020

⌛ Testing commit 7dad29d with merge 7402a39...

@bors
Copy link
Collaborator

bors commented Sep 13, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing 7402a39 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 13, 2020
@bors bors merged commit 7402a39 into rust-lang:master Sep 13, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 13, 2020
@vandenheuvel vandenheuvel deleted the remove__paramenv__def_id branch September 13, 2020 18:29
@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Sep 17, 2020

It looks like this was a pretty major wall-time regression (up to 25%). There were some ~1% improvements too, but this regression seems to outweigh those.

It looks like that was mostly in match checking, which is somewhat surprising -- there doesn't seem to be significant modifications to that code in this PR.

I suspect we'll want to revert as this is a pretty big regression, but it would be useful to try to analyze where the instruction diff is coming from.

@jackh726
Copy link
Member

jackh726 commented Sep 17, 2020

@Mark-Simulacrum that's really strange, not what I would expect at all. Seems those 2-3% regressions in instruction count for unicode_normalization are pretty significant.

I wonder if the additional PredicateAtom variant is the problem? It might be worth doing a revert+that change alone for a perf comparison.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Sep 17, 2020

Ah, I missed that there's two things being done here -- would've been good to split that into separate commits perhaps. @vandenheuvel do you think you could prepare a PR that reverts this one, and then just adds the PredicateAtom variant as @jackh726 suggests?

@jackh726
Copy link
Member

jackh726 commented Sep 17, 2020

@Mark-Simulacrum in hindsight, yeah, splitting into two makes sense. But given that we didn't expect a perf regression (since we were more focused on the removal of DefId from ParamEnv, it didn't seem necessary. (Only adding the PredicateAtom variant is basically useless without also removing DefId from ParamEnv.)

If @vandenheuvel can't, I can try to make a PR this weekend for perf.

@Mark-Simulacrum
Copy link
Member

Mark-Simulacrum commented Sep 17, 2020

No worries! Yeah, we'll check in on this next week's perf triage (roughly Tuesday) but it's no big deal if it takes some time.

@vandenheuvel vandenheuvel mentioned this pull request Sep 19, 2020
Merged
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Sep 21, 2020

Hi! This PR showed up in the weekly perf triage report. It resulted in a small improvement across the board in instruction counts. However, some benchmarks, notably unicode-normalization regressed moderately.

Seems like this is already being looked into. Thanks all!

@vandenheuvel
Copy link
Contributor Author

vandenheuvel commented Sep 22, 2020

@ecstatic-morse this issue is now picked up in #77058.

@ecstatic-morse ecstatic-morse mentioned this pull request Sep 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jackh726 jackh726 jackh726 requested changes

@nikomatsakis nikomatsakis nikomatsakis approved these changes

+1 more reviewer

@detrumi detrumi detrumi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@jackh726 jackh726

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.