Skip to content

Introduce cache for projection #32287

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

Conversation

nikomatsakis
Copy link
Contributor

Two main changes:

  1. Introduce a simple cache for projection
  2. Refactor projection candidate selection so that it runs select inside a probe

Both of these changes arose from work I've been doing extending @soltanmm's branch to handle lazy normalization. The cache is still incomplete in my mind, but it is certainly handy: for example it solves the explosion in compilation time for #31849.

Also, the refactoring to use select inside a probe does tweak the heuristics very slightly, in that candidates from being an object type now have a lower precedence than where-clause candidates. I wouldn't expect this to matter in practice. If it did, we could pull that logic for detecting object types more into project, but I'd prefer not to.

I'll try to write-up further thoughts for improving this cache (and lazy norm etc) elsewhere. There are two main improvements, listed briefly here:

  1. When we introduce a type variable, only do that once. Should be a straight-forward extension.
  2. I'd like to skolemize away regions and store "higher-ranked" cache keys and values (we might even use the same approach for unbound type variables eventually). This will be easier once we push more on the plumbing that @soltanmm has been working on.

Fixes #31849.

cc @arielb1, @aturon, @asajeffrey

We ought not to be affecting inference state when assembling candidates,
so invoke select inside of a probe.
The projection cache is snapshottable. This is because it will store
projections out of type variables and other region data that may well go
stale. This is different from (e.g.) the selection cache, which uses
freshened keys.
@rust-highfive
Copy link
Contributor

r? @jroesch

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

@nikomatsakis
Copy link
Contributor Author

r? @arielb1

@rust-highfive rust-highfive assigned arielb1 and unassigned jroesch Mar 16, 2016
@nikomatsakis
Copy link
Contributor Author

Oops, forgot that I had to rebase this over the specialization code. Let me fix that.

@asajeffrey
Copy link

Once this is rebased, I'll give it a shot on my horrorcode.

Before we would ignore a candidate if it happened to be an impl with a
default type. This change makes us never add the impl in the first
place. This seems largely equivalent, though there might be some subtle
difference in that -- before -- we would have failed to normalize if
there was a "trait-def-candidate" contending with an (opaque) impl
candidate. This corresponds I guess to a case like `<<A as Trait>::B as
Trait2>::C`, and the definition of `Trait` contains a clause. Pretty
obscure, but it seems like it's... ok to favor the trait definition in
such a case.
@nikomatsakis
Copy link
Contributor Author

@asajeffrey I think it builds now, go for it

@asajeffrey
Copy link

@nikomatsakis will do!

@nikomatsakis
Copy link
Contributor Author

@asajeffrey give me a second to resolve the travis failures, which were legit

@asajeffrey
Copy link

@nikomatsakis ok, let me know when I'm good to go

@nikomatsakis
Copy link
Contributor Author

@asajeffrey ok fixed (afaik)

@asajeffrey
Copy link

@nikomatsakis ok, starting the build...

@asajeffrey
Copy link

Hmm, with this PR I now get a type error on my code :( (https://github.com/asajeffrey/wasm)

$ PATH=/home/ajeffrey/github/rust-lang/rust/x86_64-unknown-linux-gnu/stage2/bin/:$PATH cargo build
   Compiling wasm_sexpr v0.0.1 (file:///home/ajeffrey/github/asajeffrey/wasm/wasm-sexpr)
src/lexer.rs:194:26: 194:40 error: type mismatch resolving `for<'a> <fn(core::option::Option<char>) -> core::result::Result<lexer::Token<'a>, lexer::LexError> {lexer::mk_unexpected_char_err} as parsell::Function<core::option::Option<char>>>::Output == core::result::Result<lexer::Token<'a>, lexer::LexError>`:
 expected bound lifetime parameter 'a,
    found concrete lifetime [E0271]
src/lexer.rs:194         WASM_TOKEN.boxed(mk_lexer_state).init(data)
                                          ^~~~~~~~~~~~~~
src/lexer.rs:194:26: 194:40 help: run `rustc --explain E0271` to see a detailed explanation
src/lexer.rs:194:26: 194:40 note: required by `lexer::mk_lexer_state`
error: aborting due to previous error
Could not compile `wasm_sexpr`.

@nikomatsakis
Copy link
Contributor Author

@asajeffrey cool, let me take a look.

@nikomatsakis
Copy link
Contributor Author

@nikomatsakis
Copy link
Contributor Author

ok, I think I see the problem, seems a bit obvious in retrospect -- for some reason I thought that I was handling higher-ranked things correctly, but I think I am ... just not. let me ponder best fix. (some of the longer-term improvements I had in mind would I think address this, maybe worth trying to do them now)

@nikomatsakis
Copy link
Contributor Author

Closing for now while I ponder the best solution here :)

@asajeffrey
Copy link

OK, good luck!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants