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

Arbitrary self types v2: probe for more methods, take two #128548

Conversation

adetaylor
Copy link
Contributor

This is a new version of #127812. Here's what I said last time, which still applies:

This PR is one of the first steps towards arbitrary self types v2, RFC 3519 (#44874). Specifically, it is step 2 of the plan outlined here. That says:

  1. Search for potentially conflicting method candidates per the deshadowing part of the RFC. This is the riskiest part - per
    > the production of errors requires us to interrogate more candidates to look for potential conflicts, so this could have a compile-time performance penalty which we should measure
    let's attempt to land this first and see if performance issues show up. If so, it's back to the drawing board.

So - this commit isn't useful in itself but we need to determine its performance and any unexpected compatibility problems. That's what this PR is for. We shouldn't necessarily merge it.

Here's what this PR does.

Rust prioritizes method candidates in this order:

  1. By value;
  2. By reference;
  3. By mutable reference;
  4. By const ptr.

Previously, if a suitable candidate was found in one of these earlier categories, Rust wouldn't even move onto probing the other categories.

As part of the arbitrary self types work, we're going to need to change that - even if we choose a method from one of the earlier categories, we will sometimes need to probe later categories to search for methods that we may be shadowing.

This commit adds those extra searches for shadowing, but it does not yet produce an error when such shadowing problems are found. That will come in a subsequent commit, by filling out the 'check_for_shadowing' method.

We're adding the extra searches in a preliminary commit because they are the risky bit. We want to find any functional or performance problems from these extra searches before we proceed with the actual work on arbitrary self types and specifically on extra deshadowing errors.

--

What's different this time relative to #127812? Well, this is a more invasive change to the probing algorithm which hopefully minimizes the amount of work done during the extra probing for shadowed methods. Specifically, we only need to warn about potentially shadowed methods if three conditions are true:

  • the shadowed method is not the same as the shadower (different DefIds)
  • the potential shadower involves the same self type as the shadowed method
  • the potential shadower has been found at a different position along the chain of Receiver traits than the shadowed.

Previously, we checked these conditions only when a potentially shadowed pick was found. We now apply them during the search process, which should minimize the amount of work required.

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

r? @BoxyUwU

rustbot has assigned @BoxyUwU.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 2, 2024
@adetaylor
Copy link
Contributor Author

adetaylor commented Aug 2, 2024

The first commit in this PR has been raised separately as #128549 since it probably seems a good thing to land anyway, for a slight code simplification.

@adetaylor
Copy link
Contributor Author

r? @wesleywiser

though it's still a draft PR and not quite ready for review yet. In any case the purpose is performance testing, I'm not saying this is mergable quality.

@rust-log-analyzer

This comment has been minimized.

@adetaylor adetaylor force-pushed the receiver_trait_with_target_deshadowing_probes_v2 branch 2 times, most recently from 8649ead to 6e830c3 Compare August 2, 2024 15:28
@rust-log-analyzer

This comment has been minimized.

@adetaylor adetaylor force-pushed the receiver_trait_with_target_deshadowing_probes_v2 branch from 6e830c3 to ac957e3 Compare August 7, 2024 16:10
Rust prioritizes method candidates in this order:
1. By value;
2. By reference;
3. By mutable reference;
4. By const ptr.

Previously, if a suitable candidate was found in one of these earlier
categories, Rust wouldn't even move onto probing the other categories.

As part of the arbitrary self types work, we're going to need to change
that - even if we choose a method from one of the earlier categories, we
will sometimes need to probe later categories to search for methods that
we may be shadowing.

This commit adds those extra searches for shadowing, but it does not yet
produce an error when such shadowing problems are found. That will come
in a subsequent commit, by filling out the 'check_for_shadowing'
method.

We're adding the extra searches in a preliminary commit because they are
the risky bit. We want to find any functional or performance problems
from these extra searches before we proceed with the actual work on
arbitrary self types and specifically on extra deshadowing errors.
A subsequent commit will involve working out which methods actually
shadow other methods, and one of the rules will be to compare the depth
via a chain of Receiver traits of the shadowing and shaodwed methods.
This commit records this information; currently unused.
This commit applies constraints to the searches we perform when
searching for potential shadows, in order to make that search quicker.
@adetaylor adetaylor force-pushed the receiver_trait_with_target_deshadowing_probes_v2 branch from ac957e3 to 7f90c11 Compare August 7, 2024 17:20
@adetaylor
Copy link
Contributor Author

I don't know if I have permission to do this, let's see:

@rust-timer build 7f90c11

@rust-timer

This comment has been minimized.

@adetaylor
Copy link
Contributor Author

Well, my dear bot, that's what I thought you'd say :) In that case @wesleywiser I am marking this as "ready for review" but as noted, it's not really mergable-quality yet, it's more to do a performance test to determine if this approach to deshadowing is feasible without significant performance penalty (if so, I will clean things up before merging). So if you wouldn't mind redoing this that'd be much appreciated.

@adetaylor adetaylor marked this pull request as ready for review August 8, 2024 14:15
@lqd
Copy link
Member

lqd commented Aug 8, 2024

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
…_deshadowing_probes_v2, r=<try>

Arbitrary self types v2: probe for more methods, take two

This is a new version of rust-lang#127812. Here's what I said last time, which still applies:

This PR is one of the first steps towards arbitrary self types v2, RFC [3519](rust-lang/rfcs#3519) (rust-lang#44874). Specifically, it is [step 2 of the plan outlined here](rust-lang#44874 (comment)). That says:

> 1. **Search for potentially conflicting method candidates** per the [deshadowing part of the RFC](https://github.com/rust-lang/rfcs/blob/master/text/3519-arbitrary-self-types-v2.md#compiler-changes-deshadowing). This is the riskiest part - per
    > the production of errors requires us to interrogate more candidates to look for potential conflicts, so this could have a compile-time performance penalty which we should measure
>    let's attempt to land this first and see if performance issues show up. If so, it's back to the drawing board.

So - this commit isn't useful in itself but we need to determine its performance and any unexpected compatibility problems. That's what this PR is for. We shouldn't necessarily merge it.

Here's what this PR does.

Rust prioritizes method candidates in this order:
1. By value;
2. By reference;
3. By mutable reference;
4. By const ptr.

Previously, if a suitable candidate was found in one of these earlier categories, Rust wouldn't even move onto probing the other categories.

As part of the arbitrary self types work, we're going to need to change that - even if we choose a method from one of the earlier categories, we will sometimes need to probe later categories to search for methods that we may be shadowing.

This commit adds those extra searches for shadowing, but it does not yet produce an error when such shadowing problems are found. That will come in a subsequent commit, by filling out the 'check_for_shadowing' method.

We're adding the extra searches in a preliminary commit because they are the risky bit. We want to find any functional or performance problems from these extra searches before we proceed with the actual work on arbitrary self types and specifically on extra deshadowing errors.

--

What's different this time relative to rust-lang#127812? Well, this is a more invasive change to the probing algorithm which hopefully minimizes the amount of work done during the extra probing for shadowed methods. Specifically, we only need to warn about potentially shadowed methods if three conditions are true:

* the shadowed method is not the same as the shadower (different `DefId`s)
* the potential shadower involves the same `self` type as the shadowed method
* the potential shadower has been found at a different position along the chain of `Receiver` traits than the shadowed.

Previously, we checked these conditions only when a potentially shadowed pick was found. We now apply them during the search process, which should minimize the amount of work required.
@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Trying commit 7f90c11 with merge 317a31d...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2024
…_deshadowing_probes_v2, r=<try>

Arbitrary self types v2: probe for more methods, take two

This is a new version of rust-lang#127812. Here's what I said last time, which still applies:

This PR is one of the first steps towards arbitrary self types v2, RFC [3519](rust-lang/rfcs#3519) (rust-lang#44874). Specifically, it is [step 2 of the plan outlined here](rust-lang#44874 (comment)). That says:

> 1. **Search for potentially conflicting method candidates** per the [deshadowing part of the RFC](https://github.com/rust-lang/rfcs/blob/master/text/3519-arbitrary-self-types-v2.md#compiler-changes-deshadowing). This is the riskiest part - per
    > the production of errors requires us to interrogate more candidates to look for potential conflicts, so this could have a compile-time performance penalty which we should measure
>    let's attempt to land this first and see if performance issues show up. If so, it's back to the drawing board.

So - this commit isn't useful in itself but we need to determine its performance and any unexpected compatibility problems. That's what this PR is for. We shouldn't necessarily merge it.

Here's what this PR does.

Rust prioritizes method candidates in this order:
1. By value;
2. By reference;
3. By mutable reference;
4. By const ptr.

Previously, if a suitable candidate was found in one of these earlier categories, Rust wouldn't even move onto probing the other categories.

As part of the arbitrary self types work, we're going to need to change that - even if we choose a method from one of the earlier categories, we will sometimes need to probe later categories to search for methods that we may be shadowing.

This commit adds those extra searches for shadowing, but it does not yet produce an error when such shadowing problems are found. That will come in a subsequent commit, by filling out the 'check_for_shadowing' method.

We're adding the extra searches in a preliminary commit because they are the risky bit. We want to find any functional or performance problems from these extra searches before we proceed with the actual work on arbitrary self types and specifically on extra deshadowing errors.

--

What's different this time relative to rust-lang#127812? Well, this is a more invasive change to the probing algorithm which hopefully minimizes the amount of work done during the extra probing for shadowed methods. Specifically, we only need to warn about potentially shadowed methods if three conditions are true:

* the shadowed method is not the same as the shadower (different `DefId`s)
* the potential shadower involves the same `self` type as the shadowed method
* the potential shadower has been found at a different position along the chain of `Receiver` traits than the shadowed.

Previously, we checked these conditions only when a potentially shadowed pick was found. We now apply them during the search process, which should minimize the amount of work required.
@bors
Copy link
Contributor

bors commented Aug 8, 2024

⌛ Trying commit 7f90c11 with merge aa049af...

@bors
Copy link
Contributor

bors commented Aug 8, 2024

☀️ Try build successful - checks-actions
Build commit: aa049af (aa049af73ab994dcba850292ad20930a9596d802)

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (aa049af): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.3% [0.2%, 0.3%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

Results (primary 0.5%, secondary -3.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.2% [0.7%, 3.7%] 2
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-2.8% [-2.8%, -2.8%] 1
Improvements ✅
(secondary)
-3.2% [-3.2%, -3.2%] 1
All ❌✅ (primary) 0.5% [-2.8%, 3.7%] 3

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 760.03s -> 763.804s (0.50%)
Artifact size: 337.08 MiB -> 337.09 MiB (0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 8, 2024
@adetaylor
Copy link
Contributor Author

@wesleywiser Hello. Can you suggest someone who might be able to help me interpret these perf results and determine whether they're "OK enough"?

My interpretation is:

If this looks OK to you, I'll regard step 2 of this plan as completed then move onto step 3. There's no need to merge this PR.

@wesleywiser
Copy link
Member

I think the performance results are fine! As you say, there is only one benchmark deemed "significant", it's a secondary benchmark and recent runs on that benchmark have some amount of variance anyway.

@adetaylor
Copy link
Contributor Author

Thanks - closing this PR then, and I'll move onto the next step!

@adetaylor adetaylor closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants