Uh oh!
There was an error while loading. Please reload this page.
wf: handle "livelock" checking before reaching WfPredicates::compute. - #70170
Conversation
eddyb
commented
Mar 19, 2020
@bors try @rust-timer queue (there may be new noop |
rust-timer
commented
Mar 19, 2020
Awaiting bors try build completion |
bors
commented
Mar 19, 2020
⌛ Trying commit 1b15eb9a35ef930ba373fc945ee84dcabededdff with merge 05f512090ead730e044e52571dba2da5ff8f9dcf... |
eddyb
commented
Mar 20, 2020
(I forgot to push some comment tweaks, hopefully it doesn't kill the try build) |
eddyb
commented
Mar 20, 2020
@rust-timer build 05f512090ead730e044e52571dba2da5ff8f9dcf |
rust-timer
commented
Mar 20, 2020
Queued 05f512090ead730e044e52571dba2da5ff8f9dcf with parent f4c675c, future comparison URL. |
rust-timer
commented
Mar 20, 2020
Finished benchmarking try commit 05f512090ead730e044e52571dba2da5ff8f9dcf, comparison URL. |
eddyb
commented
Mar 20, 2020
The worst regression, I guess I'll try to come up with a synthetic stress test that hits the worst case of this PR ( But still, 7.5% regression of |
bors
commented
Mar 21, 2020
☔ The latest upstream changes (presumably #70205) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Wait I forgot about this, this is technically a bugfix (before, if compute returned false here, those inference variables would be forgotten about).
So it might make sense that it's slower. Let me, uh, make a PR that only changes this, and check that for perf regressions.
EDIT: Actually, there's the entirety of #70168, predicate_obligations being the other user of .compute w/o checking the return value. I might be able to pinpoint where the extra obligations are coming from.
There was a problem hiding this comment.
This is it.
typeck_tables_of on clap-rs gets slightly faster (instead of being a significant regression), if I ignore upvar types that are ty::Infer(ty::TyVar(_)) and don't resolve.
But that's a WF hole, right? Do we just accept the regression?
There was a problem hiding this comment.
Sorry, I lost track of this PR. I agree that this is a bug-fix.
eddyb
commented
Apr 3, 2020
I added some fast paths, let's see if they improve anything (but also see #70170 (comment)): @bors try @rust-timer queue |
rust-timer
commented
Apr 3, 2020
Awaiting bors try build completion |
bors
commented
Apr 3, 2020
⌛ Trying commit ef83aa34b11d92b10f53b514d04a5c2b475707d2 with merge bb0cbcbd0a0e408606210fa280cf86c90448764c... |
bors
commented
Apr 3, 2020
☀️ Try build successful - checks-azure |
rust-timer
commented
Apr 3, 2020
Queued bb0cbcbd0a0e408606210fa280cf86c90448764c with parent 424c793, future comparison URL. |
rust-timer
commented
Apr 3, 2020
Finished benchmarking try commit bb0cbcbd0a0e408606210fa280cf86c90448764c, comparison URL. |
Hehe, I think my second commit, I'm not surprised, there were a lot of macro-replicated integer literals in that code. |
nikomatsakis
commented
Apr 21, 2020
I'm inclined to merge this PR @eddyb -- r=me on the code itself -- the clap-rs-debug perf regression seems small and I agree it's a bug fix, and there are some significant (albeit somewhat surprising?) improvements, too. I guess your explanation makes sense, though. |
eddyb
commented
May 1, 2020
Let's get some new numbers, post-rust-lang/rustc-perf#645: @bors try @rust-timer queue |
rust-timer
commented
May 1, 2020
Awaiting bors try build completion |
bors
commented
May 1, 2020
⌛ Trying commit 05a872d with merge ad57f0a2528d29aca128300e3fa1b174a9eb497e... |
bors
commented
May 1, 2020
☀️ Try build successful - checks-azure |
rust-timer
commented
May 1, 2020
Queued ad57f0a2528d29aca128300e3fa1b174a9eb497e with parent e94eaa6, future comparison URL. |
rust-timer
commented
May 1, 2020
Finished benchmarking try commit ad57f0a2528d29aca128300e3fa1b174a9eb497e, comparison URL. |
eddyb
commented
May 1, 2020
@bors r=nikomatsakis rollup=never |
bors
commented
May 1, 2020
📌 Commit 05a872d has been approved by |
bors
commented
May 2, 2020
bors
commented
May 2, 2020
☀️ Test successful - checks-azure |
For
wf::obligations's "livelock" handling, this PR shouldn't cause any behavioral changes, as the check moved to it should be equivalent to the old one inWfPredicates::compute.However, it fixes#70168 by making other users of
WfPredicates::compute(that is,wf::predicate_obligationsandcompute's own upvar handling) correct forty::Infer, in that they now get aWellFormed(ty::Infer(_))obligation instead of silently ignoring the type.r? @nikomatsakis