Refactor method dispatch infrastructure #18694
Conversation
|
cc @gankro, this solves your particular example (it's one of the tests I added, in fact) |
|
cc @aturon this implements the scheme we discussed |
| }; | ||
| let x: [int, ..4] = [1,2,3,4]; | ||
| let xptr = x.as_slice() as *const _; | ||
| xptr.foo(); |
There was a problem hiding this comment.
Coercion/cast rules are changing, and this is a weird psuedo-ambiguous case. That is, slices will cast to both *const [T] and *const T. Might want to disambiguate here. Might also just want to test that xptr.foo() and xsliceptr.foo() both work, in case there's a symmetry issue.
There was a problem hiding this comment.
So, a follow-up PR should address the fact that, currently, method resolution does not consider coercions but only subtyping. (More accurately, the rewrite in this PR does not and the original method code did not; the current code does, and I think we will want to reproduce that logic, but I didn't want to merge it all into one shot.)
In any case, adding more tests seems like a good idea.
|
r? @nick29581 (feel free to let me know if you'd rather not, but it seems like this will interact with coercion stuff, so it'd be good to look at it) |
| @@ -3610,7 +3606,7 @@ fn check_expr_with_unifier(fcx: &FnCtxt, | |||
| lvalue_pref: LvaluePreference, | |||
| base: &ast::Expr, | |||
| field: &ast::SpannedIdent, | |||
| tys: &[P<ast::Ty>]) { | |||
| _tys: &[P<ast::Ty>]) { | |||
There was a problem hiding this comment.
If we're no longer using tys, why keep it around?
| Finally, to actually pick the method, we will search down the steps, | ||
| trying to match the receiver type against the candidate types. At | ||
| step, we also consider an auto-ref and auto-mut-ref to see whether |
|
r=me with the various nits addressed |
|
Needs a couple of |
nikomatsakis
force-pushed
the
issue-18208-method-dispatch-2
branch
from
d5e948b to
9369a07
Compare
|
Huh. So this passes locally. (Just retested last night.) I'm going to try a non-optimized build locally. @alexcrichton @brson does this failure look familiar (you're my go to "bors trouble" people)? http://buildbot.rust-lang.org/builders/auto-linux-32-nopt-c/builds/2154 Seems like it could be legit so I'm reluctant to |
|
Hmm, this passed locally both with and without optimizations. |
|
This looks like #18777 I believe. |
…2, r=nrc This is a pretty major refactoring of the method dispatch infrastructure. It is intended to avoid gross inefficiencies and enable caching and other optimizations (e.g. #17995), though it itself doesn't seem to execute particularly faster yet. It also solves some cases where we were failing to resolve methods that we theoretically should have succeeded with. Fixes #18674. cc #18208
|
Looks like this also fixes #18783 |
|
Needs rebase again. |
nikomatsakis
force-pushed
the
issue-18208-method-dispatch-2
branch
2 times, most recently
from
f68501f to
9ba6a6e
Compare
|
Rebased. I also removed the debugging commit which was probably exacerbating memory usage. |
|
Measurement with valgrind suggests that this comment significantly drops the memory consumption (outside of LLVM, of course). |
|
Actually my measurement was in error. I am going to rerun. |
|
@pcwalton I ported the quick reject mechanism onto this branch. It definitely helps with execution time (~50% reduction in type-checking, as you found) as well as memory usage. I found peak memory usage to be as follows: I did not measure memory usage (yet) with quick-reject integrated into trait dispatch. My version of quick reject is significantly simpler than yours; I excluded all the operator overloading support. The reason for this was that the new operator dispatch means that looking up overloaded operators is just a trait lookup, and that already has a cache and (now) quick reject integration as well. Also I wanted to port just a bit at a time, and this initial version seemed to show the same benefits as you measured for the full version. Anyway, we could port the overloaded operator filters later. Seeing as how this is a port of code you wrote, which means there have been at least 2 sets of eyes on it, I'm not sure if review is required, but to be on the safe side: r? @pcwalton for the final 2 commits. |
|
Ah, I remembered now the other reason I left out most of the operator filters: with the exception of overloaded deref (and to some extent |
|
@nikomatsakis it looks like it needs a rebase as I get the following error: |
|
Looks like is related to #18752 |
|
r=me for the quick reject bits |
nikomatsakis
force-pushed
the
issue-18208-method-dispatch-2
branch
from
2009f85 to
7ea4695
Compare
groundwork for better performance. Key points: - Separate out determining which method to use from actually selecting a method (this should enable caching, as well as the pcwalton fast-reject strategy). - Merge the impl selection back into method resolution and don't rely on trait matching (this should perform better but also is needed to resolve some kind of conflicts, see e.g. `method-two-traits-distinguished-via-where-clause.rs`) - Purge a lot of out-of-date junk and coercions from method lookups.
…arameters too when creating xform-self-type. Fixes rust-lang#18208.
… quickly throwing out method candidates. Yields a 40%-50% improvement in typechecking time as well as lowering peak memory use from 2.2GB to 1.8GB (due to creating fewer types). Conflicts: src/librustc/driver/config.rs src/librustc/middle/ty.rs src/librustc/middle/typeck/check/method.rs src/librustc/middle/typeck/check/mod.rs src/librustc/middle/typeck/coherence/mod.rs
yield an incremental improvement (type-checking rustc drops from ~9s to ~8s).
nikomatsakis
force-pushed
the
issue-18208-method-dispatch-2
branch
from
7ea4695 to
99fbd34
Compare
…2, r=nrc This is a pretty major refactoring of the method dispatch infrastructure. It is intended to avoid gross inefficiencies and enable caching and other optimizations (e.g. #17995), though it itself doesn't seem to execute particularly faster yet. It also solves some cases where we were failing to resolve methods that we theoretically should have succeeded with. Fixes #18674. cc #18208
internal: Show mir eval errors on hover with debug env var set
This is a pretty major refactoring of the method dispatch infrastructure. It is intended to avoid gross inefficiencies and enable caching and other optimizations (e.g. #17995), though it itself doesn't seem to execute particularly faster yet. It also solves some cases where we were failing to resolve methods that we theoretically should have succeeded with.
Fixes #18674.
cc #18208