Here's a proposal for solving some of the duplication problems, by adding a context shared by the components of the pipeline https://github.com/rust-lang/polonius/pull/134
While it also allowed me to remove some cloning here and there, it mostly avoids computing initialization and liveness twice in the worst case of using the
Hybrid variant with potential errors found in the pre-pass.
It also helps pave the way to using these potential errors to heavily prune the data the following variant uses (and which we have prototyped and know is a big optimization boost, but can't open a PR right now as it could hinder the illegal subset relations errors work)
let me know what you think :)
I’ll look more into it when I have time but COOL
Also, about the leapjoins to joins: I thought leapjoins were an optimised form?!
(when joining more than one predicate yeah. if you have only two, it's good to try both but regular joins should just do less work in that case — and and if the relations happen to have the same key, you wouldn't even save an index, which is the other factor to take into account: sometimes a regular join + the index maintenance it'd require is more costly on bigger relations than a leapjoin on two relations)
Ok so my intuition wasn’t entirely off
yes, and in this case it doesn't matter much, a difference of 1% (while I remember once trying a leapjoin in LocationInsensitive where it was a 7% pessimization)
@Albin Stjerna I've pushed the latest changes: stop using allfacts altogether, and divide it in different contexts for each step (which also allowed cleaning up the signatures of initialization/liveness). No step can use another step's data, or overwrite it, etc.
Nice! I'll have a look. :)
@Albin Stjerna we can't remove the clones without rustc giving us ownership of all_facts
some maybe, but not all
Is this recent? I remember moving out some stuff and that worked fine
But ok I see
nope, since always
you moved them out of the initial allfacts clone
Aaaaah ok I see
That explains it
compute cloned it before passing it to the variants
this is still done, but piecewise for each step context
we may take care of that at the same time as updating rustc to the T: FactTypes API, if we indeed plan to go on with that
(btw all the data is Copy/Clone so we could technically "remove the clones" but that wouldn't change much here: we'll want to remove the allocations)