Stream: t-compiler/wg-polonius

Topic: context sharing #134


lqd (Oct 12 2019 at 14:49, on Zulip):

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

lqd (Oct 12 2019 at 14:49, on Zulip):

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.

lqd (Oct 12 2019 at 14:50, on Zulip):

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)

lqd (Oct 12 2019 at 14:50, on Zulip):

let me know what you think :)

Albin Stjerna (Oct 13 2019 at 06:18, on Zulip):

I’ll look more into it when I have time but COOL

Albin Stjerna (Oct 13 2019 at 06:19, on Zulip):

Also, about the leapjoins to joins: I thought leapjoins were an optimised form?!

lqd (Oct 13 2019 at 10:10, on Zulip):

(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)

Albin Stjerna (Oct 13 2019 at 15:55, on Zulip):

Ok so my intuition wasn’t entirely off

lqd (Oct 13 2019 at 17:48, on Zulip):

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)

lqd (Oct 14 2019 at 11:48, on Zulip):

@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.

Albin Stjerna (Oct 14 2019 at 11:57, on Zulip):

Nice! I'll have a look. :)

lqd (Oct 14 2019 at 12:09, on Zulip):

@Albin Stjerna we can't remove the clones without rustc giving us ownership of all_facts

Albin Stjerna (Oct 14 2019 at 12:09, on Zulip):

Aah ok

lqd (Oct 14 2019 at 12:09, on Zulip):

some maybe, but not all

Albin Stjerna (Oct 14 2019 at 12:10, on Zulip):

Is this recent? I remember moving out some stuff and that worked fine

Albin Stjerna (Oct 14 2019 at 12:10, on Zulip):

But ok I see

lqd (Oct 14 2019 at 12:10, on Zulip):

nope, since always

lqd (Oct 14 2019 at 12:10, on Zulip):

you moved them out of the initial allfacts clone

Albin Stjerna (Oct 14 2019 at 12:10, on Zulip):

Aaaaah ok I see

Albin Stjerna (Oct 14 2019 at 12:11, on Zulip):

That explains it

lqd (Oct 14 2019 at 12:11, on Zulip):

compute cloned it before passing it to the variants

lqd (Oct 14 2019 at 12:11, on Zulip):

this is still done, but piecewise for each step context

lqd (Oct 14 2019 at 12:14, on Zulip):

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

lqd (Oct 14 2019 at 12:18, on Zulip):

(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)

Last update: Nov 15 2019 at 20:50UTC