Proposal: split AllFacts into contexts dedicated to each component of the pipeline
#134
Conversation
- Introduce a context to store data common to all variants: initialization, liveness, some of the facts as Relations, so that the Hybrid variant doesn't have to perform the same steps twice - inline the hybrid variant - make variants return a Relation of errors so we can reuse them more easily in the future - store partial results in the context: the locinsensitive potential errors will be used to filter loans in the optimized variant which follows it - make liveness and initialization use and produce Relations instead of cloning them via Vecs
…instead of leapjoins that's 1% of free benchmark real estate
lqd
force-pushed
the
its_all_contextual
branch
from
112e5fb to
61d763e
Compare
|
I think this looks great, but I'm not 100% comfy with the |
|
We can split it up more for sure because of the initial clone, while I was trying not to move away too much from AllFacts but there's no specific reason we couldn't do that. We also need to remove the clone and that will require changes in rustc as well, so I think we can use this to iterate on the kind of interface we'd need (after the clone), and then fix that upstream. |
|
Sounds reasonable. I also have a few changes of the same kind I'd like to make to rustc (for example, I plan on having |
…-dedicated contexts
lqd
changed the title
AllFacts into contexts dedicated to each component of the pipeline
|
(updated this to completely separate each step's data) |
| let liveness_ctx = LivenessContext { | ||
| var_used: all_facts.var_used.clone(), | ||
| var_defined: all_facts.var_defined.clone(), | ||
| var_drop_used: all_facts.var_drop_used.clone(), | ||
| var_uses_region: all_facts.var_uses_region.clone(), | ||
| var_drops_region: all_facts.var_drops_region.clone(), | ||
| }; |
There was a problem hiding this comment.
Why do you need these clones? I thought they were only used here?
There was a problem hiding this comment.
For the same reason as the previous mem::replaces: we can't move these out of all_facts: &AllFacts<T>
This introduce contexts to store data between the pipeline's steps, to solve some of the problems described in #112.
These contexts are used to store:
This also makes more use of
Relations between initialization and liveness to avoid creating and cloning the same data (so a slight optimization here).This separation should also drop the initialization input relations after
var_maybe_initialized_on_exitis computed, and the liveness input relations afterregion_live_atis computed, instead of storing them all throughout the pipeline (although I haven't made sure of how much memory it "saves").While this PR avoids some
Relationcloning (but not all, as it'd require changes in concert with rustc, and which will be done in the future), it mostly avoids computing initialization and liveness twice as happens today, when using theHybridvariant and theLocationInsensitivepre-pass returns a potential error in the code. (These first 2 steps dominate runtime on bigger benchmarks when that happens)The
Hybridvariant also stores the potential errors found by theLocationInsensitivepre-pass, so that the following variant can heavily prune the data it traverses to find errors on only those specific "interesting" loans. (This is an optimization I've done elsewhere and has very good results on our small number of benchmarks, but can't upstream in a PR yet as this filtering could interfere with the ongoing work on illegal subset relations errors).While sharing
Relations is easy when used as join inputs, it may also be interesting to share the static data inserted in theVariables themselves, when appropriate (which is not that often right now) to avoid mapping and cloning some of this initial input data.This also re-enables logging from within the
poloniusbin, which I had forgotten to push to the dependencies cleanup PR.