Dataflow: Implement call context grouping to improve performance

[aschackmull]

This replaces our call context representation with a set-based representation of the allowed call edges, thus unifying equivalent call contexts. This unification means that we avoid redundant computation when a callable is reachable with several different, but equivalent call contexts.

In some cases I've observed 3000 equivalent call contexts, which will now be replaced by a single entity, and in that particular case the total tuple count for the stage was cut in half.

Commit-by-commit review encouraged. The first commit introduces the MakeSets primitive, which is then used to collapse local call contexts. Then a sequence of refactoring commits follow. And finally, the "Dataflow: Switch call context to a set representation" commit contains the implementation of and switch to fully set-based call contexts. This commit also does some reshuffling to ensure proper caching in CachedCallContextSensitivity.

[owen-mc]

Why does the total order for DataFlowCallable take the file into account, but not those for DataFlowCall or NodeRegion?

[aschackmull]

That's because we're ordering calls and regions within a specific callable body, so the ones we need to compare are always in the same file. But the callables that we need to compare can be spread across multiple files, so they benefit from the additional comparison column.
My hope is that this ordering is fairly temporary, as I'd like to get the MakeSets module turned into a QL primitive - once it's implemented in the evaluator then there's no need for these arbitrary orderings.

[owen-mc]

That makes sense. In the cases where we are only comparing within the same file, would it make sense to limit the ordering to other instances in the same file? Or do you really want to have a total ordering over all calls, say, even if we only compare ones in the same file?

[aschackmull]

A best-effort total ordering is simpler than trying to make it specifically per file - in the end it doesn't matter much, we just need some completely arbitrary ordering, and even in the cases where we fail to get this, the MakeSets module provides a fallback solution.

[hvitved]

Overall approach LGTM. A few comments/suggestions.

[hvitved]

Use final extends instead?

[aschackmull]

That feels slightly wrong to me, since a NodeRegion isn't necessarily a basic block - it could also be defined as e.g. a set of nodes guarded by a given guard. Currently the data flow side of the implementation assumes that NodeRegions are disjoint, but that doesn't have to be the case. It could conceivably be useful to define them differently if we run into guards that guard a huge number of basic blocks.

[hvitved]

But its are purely internal implementation detail, right, so we can always change it as we see fit? I merely suggested to use final extends to avoid the dummy toString definitions.

[aschackmull]

final extends needs a dummy alias instead, so there's not much saved. And it still feels wrong for the stated reason: A basic block is certainly a region of nodes, but a region of nodes is not necessarily a basic block.

[hvitved]

final extends?

[hvitved]

I see that this is needed because DispatchSets does not handle the case when there are no dispatch targets. However, I think it would be more explicit if we had something like

    private module DispatchSetsInput implements MkSetsInp {
      class Key = TCallEdge;

      class Value = Option<TCallEdge>::Option;

      Value getAValue(TCallEdge ctxEdge) {
        exists(DataFlowCall ctx, DataFlowCallable c, DataFlowCall call, DataFlowCallable tgt |
          ctxEdge = TMkCallEdge(ctx, c) and
          reducedViableImplInCallContext(call, c, ctx) |
          result.asSome() = TMkCallEdge(call, tgt) and
          viableImplInCallContextExtIn(call, ctx) = tgt
          or
          not exists(viableImplInCallContextExtIn(call, ctx)) and
          result.isNone()

        )
      }
    }

[aschackmull]

Something like that could work as well, but I chose the current version to avoid wrapping all of the DispatchSet edges in 'some'-wrappers. Also, it becomes quite tricky to get right if we stuff everything together like that - your version above e.g. fails to specify which calls have no dispatch, so an option-type doesn't carry enough information. For a given call affected by the context, there's either a non-empty set of dispatch edges or an empty set, but that empty set cannot be represented by a none value, because then we don't know what call it refers to.

[hvitved]

Right, the Option would have to apply to the Callable in TCallEdge. Let's leave it as-is then. I think this section deserves a comment then about why we need both CallSets and DispatchSets.

[aschackmull]

Overall approach LGTM. A few comments/suggestions.

All comments should be addressed now.

[aschackmull]

More comments addressed. (Including one change that had somehow slipped from the first commit).

[hvitved]

The latest DCA run for C# seems to be broken. The Ruby DCA run shows a nice speedup on gh/gh.

Perhaps it is worth running a final batch of DCA for all languages before merging?

[aschackmull]

Perhaps it is worth running a final batch of DCA for all languages before merging?

Sure. Running now.