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

cache dtorck constraints on ADTs #41485

Merged
merged 2 commits into from
Apr 23, 2017
Merged

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Apr 23, 2017

This avoids visiting the fields of all structs multiple times, improving item-bodies checking time by 10% (!).

Not sure whether we want this in 1.18 or 1.19. It's a big-ish patch, but the 10% win is very tempting.

r? @eddyb

Copy link
Member

@eddyb eddyb 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 modulo cleanup nits

@@ -1332,17 +1334,6 @@ impl<'a, 'tcx> ParameterEnvironment<'tcx> {
pub struct Destructor {
/// The def-id of the destructor method
pub did: DefId,
Copy link
Member

@eddyb eddyb Apr 23, 2017

Choose a reason for hiding this comment

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

Can this struct be replaced with just DefId?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to be more clear that this is the method def-id, rather than the impl-defid.

depth: usize,
ty: Ty<'tcx>)
-> Result<ty::DtorckConstraint<'tcx>, ErrorReported>
{
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be in librustc?

Copy link
Contributor Author

@arielb1 arielb1 Apr 23, 2017

Choose a reason for hiding this comment

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

It is used on both "local" and "remote" providers, and typeck provides only "local" things. I bet you can bypass that.

Copy link
Member

Choose a reason for hiding this comment

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

I already added something in librustc/ty/mod.rs like that (also see the driver). We'll clean it up when we have more uniform intercrate caching.

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 23, 2017

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 23, 2017

📌 Commit 6d01b88 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Apr 23, 2017

⌛ Testing commit 6d01b88 with merge 3004380...

@bors
Copy link
Contributor

bors commented Apr 23, 2017

💔 Test failed - status-travis

This avoids visiting the fields of all structs multiple times, improving
item-bodies checking time by 10% (!).
@arielb1
Copy link
Contributor Author

arielb1 commented Apr 23, 2017

stupid "refactored and didn't clean up" error

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 23, 2017

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.

@bors
Copy link
Contributor

bors commented Apr 23, 2017

📌 Commit 6d01b88 has been approved by eddyb

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 23, 2017

@bors r-

@arielb1
Copy link
Contributor Author

arielb1 commented Apr 23, 2017

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 23, 2017

📌 Commit d3476f4 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Apr 23, 2017

⌛ Testing commit d3476f4 with merge 2bd4b5c...

bors added a commit that referenced this pull request Apr 23, 2017
cache dtorck constraints on ADTs

This avoids visiting the fields of all structs multiple times, improving item-bodies checking time by 10% (!).

Not sure whether we want this in 1.18 or 1.19. It's a big-ish patch, but the 10% win is very tempting.

r? @eddyb
@bors bors mentioned this pull request Apr 23, 2017
5 tasks
@bors
Copy link
Contributor

bors commented Apr 23, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 2bd4b5c to master...

@bors bors merged commit d3476f4 into rust-lang:master Apr 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants