[perf] Skip attempting to run coerce_unsized on an inference variable #69530

Aaron1011 · 2020-02-27T22:57:28Z

See the included comment for a detailed explanation of why this is
sound.

See the included comment for a detailed explanation of why this is sound.

rust-highfive · 2020-02-27T22:57:32Z

r? @petrochenkov

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

Aaron1011 · 2020-02-27T23:47:26Z

Can I get a perf run?

jonas-schievink · 2020-02-27T23:49:27Z

@bors try @rust-timer queue

rust-timer · 2020-02-27T23:49:29Z

Awaiting bors try build completion

bors · 2020-02-27T23:49:39Z

⌛ Trying commit 32c360b with merge 1cdff26b98d4c2377ea6ae8929f1daebbf58f2b0...

bors · 2020-02-28T02:32:51Z

☀️ Try build successful - checks-azure
Build commit: 1cdff26b98d4c2377ea6ae8929f1daebbf58f2b0 (1cdff26b98d4c2377ea6ae8929f1daebbf58f2b0)

rust-timer · 2020-02-28T02:32:53Z

Queued 1cdff26b98d4c2377ea6ae8929f1daebbf58f2b0 with parent 6d69cab, future comparison URL.

rust-timer · 2020-02-28T06:08:25Z

Finished benchmarking try commit 1cdff26b98d4c2377ea6ae8929f1daebbf58f2b0, comparison URL.

Aaron1011 · 2020-02-28T06:15:09Z

It looks like this resulted in clean/baseline-incremental improvements across several different benchmarks, in the range of 1-1.7%.

I have absolutely no idea what's going on with syn-opt (maybe related to #69060?)

petrochenkov · 2020-02-28T08:22:20Z

r? @eddyb for review or re-assignment.

src/librustc_typeck/check/coercion.rs

eddyb · 2020-02-28T12:14:52Z

syn-opt is indeed unreliable.

Early exit looks fine to me (haven't read the comment):

source: _: Trait is always ambiguous anyway
target: we can never coerce without knowing what we're coercing to (except maybe ! but that's not an unsizing coercion)

r? @nikomatsakis for the final say

nikomatsakis · 2020-03-09T16:48:38Z

src/librustc_typeck/check/coercion.rs

+        // If either `source` or `target` is a type variable, then any applicable impl
+        // would need to be generic over the self-type (`impl<T> CoerceUnsized<SomeType> for T`)
+        // or generic over the `CoerceUnsized` type parameter (`impl<T> CoerceUnsized<T> for
+        // SomeType`).


This paragraph feels a bit misleading, although the conclusion is likely correct. In particular, if (say) the "source" type is an unbound inference variable, then it's not that the other, more narrow impls don't apply, right? It's more that any impl could apply, and hence we wind up with an ambiguous result?

That said, the trait selection code refuses to proceed (always yields ambiguous) if the source type is unknown, so this short-circuit here has no effect I imagine. I do feel like this could change behavior if the target type is unknown, since there may be at most one impl that could apply, though I feel like we probably don't want to be coercing then -- I sort of expect this coerce to only trigger if we are coercing to a "fat-pointer type", but I don't see any code that forces this to be the case.

In general, though, we don't try to coerce from cases where the source or target types are unknown anyhow, right? Well, I think that's true,but it doesn't seem to be enforced by any kind of "blanket check" in the coerce_to routine.

then it's not that the other, more narrow impls don't apply, right? It's more that any impl could apply, and hence we wind up with an ambiguous result?

When I was writing this, my concern was blanket impls - I wanted to be sure that we wouldn't bail out when we could have actually picked a blanket impl.

I do feel like this could change behavior if the target type is unknown, since there may be at most one impl that could apply

That's a good point. Since unsizing coercions happen fairly 'early' during typeck, it's possible that we might skip performing an unsizing coercion that turns out to be 'reasonable' (e.g. we later unify the target variable with the expected type).

In general, though, we don't try to coerce from cases where the source or target types are unknown anyhow, right?

I'm not quite sure what you mean.

The way I think of coercions, they trigger only when we know the source/target well enough to see that the coercion applies (otherwise we just unify). But I think that is not an 'obvious property' of the code as written.

Dylan-DPC-zz · 2020-04-17T15:39:51Z

@Aaron1011 any updates?

nikomatsakis · 2020-04-20T13:58:50Z

I guess this is blocked on me. Sorry @Aaron1011. Let me glance over it again, but I think it's fine.

nikomatsakis · 2020-04-20T13:59:35Z

@craterbot check

Let's just do a "quick check"...

craterbot · 2020-04-20T13:59:38Z

👌 Experiment pr-69530 created and queued.
🤖 Automatically detected try build 1cdff26b98d4c2377ea6ae8929f1daebbf58f2b0
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Mark-Simulacrum · 2020-04-23T11:48:00Z

FYI: Crater here won't be done for around a week and a half with current queue and since we're likely to schedule beta crater runs (which take up around a week's worth of time as well), probably closer to two to three weeks from now that this will be run through crater.

nikomatsakis · 2020-04-28T14:00:26Z

OK, I think I'm inclined to r+ instead.

The only question is whether to improve the wording of the comment.

craterbot · 2020-05-05T04:54:43Z

🚧 Experiment pr-69530 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

craterbot · 2020-05-08T11:25:43Z

🎉 Experiment pr-69530 is completed!
📊 2 regressed and 5 fixed (99934 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

nikomatsakis · 2020-05-08T12:30:19Z

Welp I never did R+ -- but we see 2 regressions, interesting.

nikomatsakis · 2020-05-08T12:30:43Z

Both spurious

nikomatsakis · 2020-05-08T12:30:47Z

@bors r+

bors · 2020-05-08T12:30:48Z

📌 Commit ff6e4ee has been approved by nikomatsakis

bors · 2020-05-08T16:42:33Z

⌛ Testing commit ff6e4ee with merge c9613c332e62aa1b5350d49ee1d8a6693c0a227f...

Dylan-DPC-zz · 2020-05-08T16:50:51Z

@bors retry yield

RalfJung · 2020-05-09T11:21:57Z

@bors rollup=never
(PR title suggests perf impact)

bors · 2020-05-09T21:01:25Z

⌛ Testing commit ff6e4ee with merge 0a3619c...

bors · 2020-05-10T00:34:48Z

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

[perf] Skip attempting to run coerce_unsized on an inference variable

32c360b

See the included comment for a detailed explanation of why this is sound.

rust-highfive assigned petrochenkov Feb 27, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2020

rust-highfive assigned eddyb and unassigned petrochenkov Feb 28, 2020

eddyb reviewed Feb 28, 2020

View reviewed changes

src/librustc_typeck/check/coercion.rs Outdated Show resolved Hide resolved

rust-highfive assigned nikomatsakis and unassigned eddyb Feb 28, 2020

Use resolved types in body of coerce_unsized

ff6e4ee

nikomatsakis reviewed Mar 9, 2020

View reviewed changes

Dylan-DPC-zz 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 Mar 29, 2020

craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 20, 2020

craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels May 8, 2020

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 May 8, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label May 10, 2020

bors merged commit 0a3619c into rust-lang:master May 10, 2020

Aaron1011 deleted the perf/skip-coerce-var branch May 10, 2020 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[perf] Skip attempting to run coerce_unsized on an inference variable #69530

[perf] Skip attempting to run coerce_unsized on an inference variable #69530

Aaron1011 commented Feb 27, 2020

rust-highfive commented Feb 27, 2020

Aaron1011 commented Feb 27, 2020

jonas-schievink commented Feb 27, 2020

rust-timer commented Feb 27, 2020

bors commented Feb 27, 2020

bors commented Feb 28, 2020

rust-timer commented Feb 28, 2020

rust-timer commented Feb 28, 2020

Aaron1011 commented Feb 28, 2020 •

edited

Loading

petrochenkov commented Feb 28, 2020

eddyb commented Feb 28, 2020

nikomatsakis Mar 9, 2020

Aaron1011 Mar 9, 2020

nikomatsakis Mar 13, 2020

Dylan-DPC-zz commented Apr 17, 2020

nikomatsakis commented Apr 20, 2020

nikomatsakis commented Apr 20, 2020 •

edited

Loading

craterbot commented Apr 20, 2020

Mark-Simulacrum commented Apr 23, 2020

nikomatsakis commented Apr 28, 2020

craterbot commented May 5, 2020

craterbot commented May 8, 2020

nikomatsakis commented May 8, 2020

nikomatsakis commented May 8, 2020

nikomatsakis commented May 8, 2020

bors commented May 8, 2020

bors commented May 8, 2020

Dylan-DPC-zz commented May 8, 2020

RalfJung commented May 9, 2020 •

edited

Loading

bors commented May 9, 2020

bors commented May 10, 2020

[perf] Skip attempting to run coerce_unsized on an inference variable #69530

[perf] Skip attempting to run coerce_unsized on an inference variable #69530

Conversation

Aaron1011 commented Feb 27, 2020

rust-highfive commented Feb 27, 2020

Aaron1011 commented Feb 27, 2020

jonas-schievink commented Feb 27, 2020

rust-timer commented Feb 27, 2020

bors commented Feb 27, 2020

bors commented Feb 28, 2020

rust-timer commented Feb 28, 2020

rust-timer commented Feb 28, 2020

Aaron1011 commented Feb 28, 2020 • edited Loading

petrochenkov commented Feb 28, 2020

eddyb commented Feb 28, 2020

nikomatsakis Mar 9, 2020

Choose a reason for hiding this comment

Aaron1011 Mar 9, 2020

Choose a reason for hiding this comment

nikomatsakis Mar 13, 2020

Choose a reason for hiding this comment

Dylan-DPC-zz commented Apr 17, 2020

nikomatsakis commented Apr 20, 2020

nikomatsakis commented Apr 20, 2020 • edited Loading

craterbot commented Apr 20, 2020

Mark-Simulacrum commented Apr 23, 2020

nikomatsakis commented Apr 28, 2020

craterbot commented May 5, 2020

craterbot commented May 8, 2020

nikomatsakis commented May 8, 2020

nikomatsakis commented May 8, 2020

nikomatsakis commented May 8, 2020

bors commented May 8, 2020

bors commented May 8, 2020

Dylan-DPC-zz commented May 8, 2020

RalfJung commented May 9, 2020 • edited Loading

bors commented May 9, 2020

bors commented May 10, 2020

Aaron1011 commented Feb 28, 2020 •

edited

Loading

nikomatsakis commented Apr 20, 2020 •

edited

Loading

RalfJung commented May 9, 2020 •

edited

Loading