Skip most of create_substs_for_ast_path if type is not generic

[bugadani]

No description provided.

[rust-highfive]

r? @varkor

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

[varkor]

Let's check whether this makes a noticeable difference.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 4ff879a95a8c3d2b5af40d0e4f4fae1d2c285da1 with merge 4ed8fc0fbdb747ed9ee9a45505ddfc72d891cc08...

[bugadani]

Given the results of my previous few PRs, I expect a huge regression, actually 😅

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: 4ed8fc0fbdb747ed9ee9a45505ddfc72d891cc08 (4ed8fc0fbdb747ed9ee9a45505ddfc72d891cc08)

[rust-timer]

Queued 4ed8fc0fbdb747ed9ee9a45505ddfc72d891cc08 with parent 22e6b9c, future comparison URL.

[rust-timer]

Finished benchmarking try commit (4ed8fc0fbdb747ed9ee9a45505ddfc72d891cc08): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[varkor]

I'll double-check this soon.

[bugadani]

There are a lot of avg >= 0 results, and no clear wins anywhere.

[varkor]

It looks like an improvement on serde, alloc and a couple of others to me. Am I misreading the results?

[bugadani]

I was under the assumption that bootstrap times aren't really representative, but perhaps that has changed since. @Mark-Simulacrum I believe you know that?

[Mark-Simulacrum]

The smaller bootstrap timings are less reliable (since it's wall times). I would not say that the bootstrap timings are unreliable, though, we usually see roughly 2-3% noise on smaller crates (<30 seconds) and ~1% noise at most on the longer crates. That's about what we're seeing here. I don't think I would call this a major improvement, but it might be a slight improvement.

[bugadani]

Hmm, thanks for the explanation. I guess I misinterpreted a different comment on a random PR.

[bugadani]

Is this not ignoring associated type bindings?

I'm not entirely certain, but as far as I can see, every trait or trait ref should have a Self˙ generic param by default, so generic_params.params.len() =/= 0 in this case. This means I don't think I can conjure up a test that fails with the older PR code but not with the new. I'm not sure if it's possible to specify an assoc type for something that's not a trait, though, which can change this.

Note: If I force the assoc bindings to an empty vector, I get a nice (and wrong) error message while compiling core, so I think we're well covered on the not-generic-but-has-associated-type-binding case :)

error[E0191]: the value of the associated type `Item` (from trait `Iterator`) must be specified
   --> library/core/src/iter/traits/iterator.rs:16:35
    |
16  | fn _assert_is_object_safe(_: &dyn Iterator<Item = ()>) {}
    |                                   ^^^^^^^^^^^^^^^^^^^ help: specify the associated type: `Iterator<Item = (), Item = Type>`
...
100 |     type Item;
    |     ---------- `Item` defined here

[bugadani]

@varkor a friendly ping just in case :) Let me know if my assesment sounds sane or I should try harder to find a test instead.

[varkor]

Sorry, I'll take a look soon; I've just been a bit snowed under for the past few days!

[bugadani]

No worries, there's no rush here.

[varkor]

I'm not entirely certain, but as far as I can see, every trait or trait ref should have a Self˙ generic param by default, so generic_params.params.len() =/= 0 in this case.

Ah, yes, I think you're right. In this case, it ought to be safe to skip the rest of the body in this case. So I think the previous change was fine (but we should have a comment explaining why it's safe). Sorry for the false alarm: would you be able to change it back?

[varkor]

Thanks for this, and sorry for taking so long with it!

@bors r+ rollup

[bors]

📌 Commit aebea52 has been approved by varkor

[bors]

⌛ Testing commit aebea52 with merge 72d2a7c...

[bors]

☀️ Test successful - checks-actions
Approved by: varkor
Pushing 72d2a7c to master...