Arbitrary self types v2: probe for more methods. #127812
Conversation
|
r? @wesleywiser rustbot has assigned @wesleywiser. Use |
|
@wesleywiser Hello, you've been assigned as reviewer! However, rather than working towards merging this, I'd love advice about how to get Crater running over this, and how best we can assess performance implications. Any suggestions? Otherwise I'll pester some of the usual suspects who have previously been involved in arbitrary self types. |
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
4740759
to
edc2276
Compare
…_deshadowing_probes, r=<try> Arbitrary self types v2: probe for more methods. 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 commit 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.
This comment has been minimized.
This comment has been minimized.
|
(a bit of iteration here because I can't get the whole |
|
Is a crater run useful here yet? We're not emitting an extra errors, right? For sure, perf run here indicates the "minimum extra work" in the happy path though. |
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.
edc2276
to
670c724
Compare
I've found that the extra calls to (for instance) |
|
@rust-timer build f313d8b |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (f313d8b): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary 3.1%)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.
CyclesResults (secondary 8.7%)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.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 700.14s -> 701.137s (0.14%) |
|
I'm glad we did a perf run :) It looks like it's significant. So, the next steps here are:
I'm unlikely to be able to work on this in the next 3 weeks or so, so I'm closing this PR for now. I may reopen it with an extra commit or two when I get back to this. |
…_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.
…_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.
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:
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 commit does.
Rust prioritizes method candidates in this order:
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.