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

Optimize dropck #64595

Merged
merged 2 commits into from
Oct 17, 2019
Merged

Optimize dropck #64595

merged 2 commits into from
Oct 17, 2019

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Sep 18, 2019

This does two things: caches the trivial_dropck check by making it a query, and shifts around the implementation of the primary dropck itself to avoid allocating many small vectors.

@rust-highfive
Copy link
Collaborator

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 18, 2019
@Mark-Simulacrum

This comment has been minimized.

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2019
@Mark-Simulacrum

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member Author

I spent some more time optimizing this and I have another commit locally (will wait on perf for first commit before pushing it) that further optimizes drop check in a pathological case to the point where drop check becomes much faster (i.e., completes to overflow).

However, that doesn't fully solve the problem as we then hit a hang when trying to print the types (somewhat unsurprisingly, these types are huge), I haven't quite figured out a good fix there.

This leads me to conclude that @pnkfelix's remarks on Zulip when discussing this (cc #4287) where apt -- we should try to come up with a check that detects this case more knowingly and errors out before we try to recurse down the chain of instantiations so we can provide better spans and such. I've not yet thought of a good way to do so, but am still thinking.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rust-timer

This comment has been minimized.

@Mark-Simulacrum

This comment has been minimized.

@rust-timer

This comment has been minimized.

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@Mark-Simulacrum

This comment has been minimized.

@bors

This comment has been minimized.

@bors

This comment has been minimized.

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit c1c16e012812695bb3f7f84ad6c6ba53b1eeccbe, comparison URL.

@Mark-Simulacrum
Copy link
Member Author

Hm, so instruction counts and such look like basically a wash -- this is somewhat expected, as we've not changed the algorithm here for most types -- however, it does show marked improvements for two test cases I examined. Both of these now complete dropck (into overflow) rather than being so slow in dropck that we don't reach overflow in reasonable time. Neither one actually completes in reasonable time due to this print which takes forever on such large types, but this PR still seems like a good thing to land (in particular, because it's not really hurting readability all that much IMO).

So I'm going to r? @pnkfelix here but feel free to reassign (we're not changing algorithm here at all so really anyone can review).

enum Perfect<T> {
    Tip(T),
    Fork(Box<Perfect<(T, T)>>),
}

fn main() {
    let _ = Perfect::Tip(Box::new(42));
}
enum Perfect<T> {
    Tip(T),
    Fork(Box<Perfect<(T, T)>>),
}

fn main() {
    let _ = Perfect::Tip(42);
}

@Mark-Simulacrum Mark-Simulacrum changed the title Make trivial dropck outlives a query Optimize dropck Sep 19, 2019
@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2019
@rust-timer
Copy link
Collaborator

Finished benchmarking try commit c1c16e012812695bb3f7f84ad6c6ba53b1eeccbe, comparison URL.

result.kinds.extend(constraints.outlives.drain(..));
result.overflows.extend(constraints.overflows.drain(..));

// At some point we just need to stop
Copy link
Member

Choose a reason for hiding this comment

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

I haven't fully brought all this back into my mental cache; can you say more here on why >= 1 is the right threshold?

If all you want to do is stop as soon as there is any overflow (which is how I interpret this), then fine. But the comment as written makes it sound like the threshold is a more interesting value, like 100 or something.

Copy link
Member

Choose a reason for hiding this comment

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

(Anyway apart from that, I think these changes look fine.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This conditional is basically because with the new code we detect thousands (and then millions.. and so on) of overflows, eventually leading to an OOM on the Perfect enum; I initially thought maybe we should have some small number, like 10, here -- but then in thinking more, I had decided that chances are, the first overflow contains all the information you need, and there's no reason for us to spin further.

I can update this comment with that summary?

@Mark-Simulacrum
Copy link
Member Author

Alright, I've updated the comment with some explanation for why we're comparing to 1 and not something larger -- I'm not sure if @pnkfelix meant #64595 (comment) as a r=me so I'll not approve this myself.

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-09-27T01:53:26.6490387Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-09-27T01:53:26.6676458Z ##[command]git config gc.auto 0
2019-09-27T01:53:26.6743765Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-09-27T01:53:26.6802161Z ##[command]git config --get-all http.proxy
2019-09-27T01:53:26.6951297Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/64595/merge:refs/remotes/pull/64595/merge
---
2019-09-27T02:02:47.6411505Z     Checking rustc_traits v0.0.0 (/checkout/src/librustc_traits)
2019-09-27T02:02:47.8141259Z error[E0412]: cannot find type `Kind` in module `ty::subst`
2019-09-27T02:02:47.8141897Z    --> src/librustc_traits/dropck_outlives.rs:234:40
2019-09-27T02:02:47.8142141Z     |
2019-09-27T02:02:47.8142452Z 234 |                 .map(|t| -> ty::subst::Kind<'tcx> { t.into() }));
2019-09-27T02:02:47.8147382Z 
2019-09-27T02:02:48.3441328Z error: aborting due to previous error
2019-09-27T02:02:48.3446105Z 
2019-09-27T02:02:48.3457816Z For more information about this error, try `rustc --explain E0412`.
---
2019-09-27T02:02:59.6882533Z == clock drift check ==
2019-09-27T02:02:59.6902885Z   local time: Fri Sep 27 02:02:59 UTC 2019
2019-09-27T02:02:59.8409457Z   network time: Fri, 27 Sep 2019 02:02:59 GMT
2019-09-27T02:02:59.8410205Z == end clock drift check ==
2019-09-27T02:03:01.0456996Z ##[error]Bash exited with code '1'.
2019-09-27T02:03:01.0513182Z ##[section]Starting: Checkout
2019-09-27T02:03:01.0515208Z ==============================================================================
2019-09-27T02:03:01.0515388Z Task         : Get sources
2019-09-27T02:03:01.0515450Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Mark-Simulacrum
Copy link
Member Author

Looks like CI broke due to changes on master, rebased -- should be fixed now.

@bors
Copy link
Contributor

bors commented Sep 28, 2019

☔ The latest upstream changes (presumably #64864) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

Ping from triage.
@Mark-Simulacrum Can you please resolve the merge conflicts?
CC @pnkfelix
Thank you.

@JohnCSimon
Copy link
Member

Pinging again from triage.
@Mark-Simulacrum Can you please resolve the merge conflicts?
CC @pnkfelix
Thank you.

This allows caching some recursive types and getting to an error much
more quickly.
Previously we'd frequently throw away vectors which is bad for
performance
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Oct 17, 2019

📌 Commit 8de7fd8 has been approved by pnkfelix

@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 Oct 17, 2019
@bors
Copy link
Contributor

bors commented Oct 17, 2019

⌛ Testing commit 8de7fd8 with merge b043380...

bors added a commit that referenced this pull request Oct 17, 2019
Optimize dropck

This does two things: caches the `trivial_dropck` check by making it a query, and shifts around the implementation of the primary dropck itself to avoid allocating many small vectors.
@bors
Copy link
Contributor

bors commented Oct 17, 2019

☀️ Test successful - checks-azure
Approved by: pnkfelix
Pushing b043380 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 17, 2019
@bors bors merged commit 8de7fd8 into rust-lang:master Oct 17, 2019
@Mark-Simulacrum Mark-Simulacrum deleted the trivial-query branch February 15, 2023 12:09
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.

7 participants