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

Conversation

@Mark-Simulacrum
Copy link
Member

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 18, 2019

r? @eddyb

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

@Mark-Simulacrum

This comment was marked as outdated.

Copy link
Member Author

Mark-Simulacrum commented Sep 18, 2019

@bors try @rust-timer queue

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Sep 18, 2019

Awaiting bors try build completion

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Sep 18, 2019

⌛️ Trying commit d19cc4b with merge c4cdd92...

bors added a commit that referenced this pull request Sep 18, 2019
Make trivial dropck outlives a query

This allows caching some recursive types and getting to an error much more quickly, though for most of those cases we'll then spend lots of time attempting to print the overflowed type. Regardless, seems like an improvement (and possibly good for perf in general).
@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member Author

Mark-Simulacrum commented Sep 18, 2019

Reassigning to myself until we get perf numbers back

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Sep 19, 2019

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 was marked as resolved.

Copy link
Contributor

bors commented Sep 19, 2019

☀️ Try build successful - checks-azure
Build commit: c4cdd92

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Sep 19, 2019

Queued c4cdd92 with parent eceec57, future comparison URL.

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Sep 19, 2019

Finished benchmarking try commit c4cdd92, comparison URL.

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member Author

Mark-Simulacrum commented Sep 19, 2019

@bors try @rust-timer queue

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Sep 19, 2019

Awaiting bors try build completion

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Sep 19, 2019

⌛️ Trying commit 5acbeac with merge aaeb5ea...

bors added a commit that referenced this pull request Sep 19, 2019
Make trivial dropck outlives a query

This allows caching some recursive types and getting to an error much more quickly, though for most of those cases we'll then spend lots of time attempting to print the overflowed type. Regardless, seems like an improvement (and possibly good for perf in general).
@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Sep 19, 2019

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.

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)

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Sep 19, 2019

💔 Test failed - checks-azure

@Mark-Simulacrum

This comment was marked as resolved.

Copy link
Member Author

Mark-Simulacrum commented Sep 19, 2019

Huh. @bors retry spurious "The user-provided path cpu-usage.csv does not exist. " hopefully?

bors added a commit that referenced this pull request Sep 19, 2019
Make trivial dropck outlives a query

This allows caching some recursive types and getting to an error much more quickly, though for most of those cases we'll then spend lots of time attempting to print the overflowed type. Regardless, seems like an improvement (and possibly good for perf in general).
@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Sep 19, 2019

⌛️ Trying commit 5acbeac with merge c1c16e0...

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Sep 19, 2019

☀️ Try build successful - checks-azure
Build commit: c1c16e0

@rust-timer

This comment was marked as resolved.

Copy link

rust-timer commented Sep 19, 2019

Queued c1c16e0 with parent 9b9d2af, future comparison URL.

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Sep 19, 2019

Finished benchmarking try commit c1c16e0, comparison URL.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Sep 19, 2019

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
@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Sep 20, 2019

Finished benchmarking try commit c1c16e0, comparison URL.

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

// At some point we just need to stop

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Sep 25, 2019

Member

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.

This comment has been minimized.

Copy link
@pnkfelix

pnkfelix Sep 25, 2019

Member

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

This comment has been minimized.

Copy link
@Mark-Simulacrum

Mark-Simulacrum Sep 25, 2019

Author Member

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 Mark-Simulacrum force-pushed the Mark-Simulacrum:trivial-query branch from 5acbeac to 21bdfb8 Sep 27, 2019
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Sep 27, 2019

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Sep 27, 2019

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 Mark-Simulacrum force-pushed the Mark-Simulacrum:trivial-query branch from 21bdfb8 to e2c80c5 Sep 27, 2019
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member Author

Mark-Simulacrum commented Sep 27, 2019

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

@bors

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

JohnCSimon commented Oct 5, 2019

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

@JohnCSimon

This comment has been minimized.

Copy link
Member

JohnCSimon commented Oct 12, 2019

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
@Mark-Simulacrum Mark-Simulacrum force-pushed the Mark-Simulacrum:trivial-query branch from e2c80c5 to 8de7fd8 Oct 12, 2019
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 17, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Oct 17, 2019

📌 Commit 8de7fd8 has been approved by pnkfelix

@bors

This comment has been minimized.

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

This comment has been minimized.

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 label Oct 17, 2019
@bors bors merged commit 8de7fd8 into rust-lang:master Oct 17, 2019
5 checks passed
5 checks passed
homu Test successful
Details
pr Build #20191012.14 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.