Skip to content

Guard reachability is rebuilt per caller-walk entry rather than once per caller #863

Description

@khatchad

Guard reachability is recomputed on every entry to the caller walk rather than once per caller, and the memoisation above it does not deduplicate the work. Whether that matters is unmeasured; the reason it is worth recording is the correctness question attached to the obvious fix.

What Is Rebuilt

The caller walk, getCallerInvokes, opens with a local map:

Map<CGNode, Set<ISSABasicBlock>> reachableByCaller = HashMapFactory.make();

That map is filled by computeReachableBlocksUnderBindings, which is a worklist over the CALLER'S WHOLE control-flow graph from its entry block, calling the constant-folding resolver at every two-successor conditional to decide whether a branch prunes.

The map is local, so it deduplicates within one call and is rebuilt on the next.

The Memo Layer Does Not Cover It

What the worklist memoises is a generator EVALUATION keyed on its query, so a repeated demand for the same query never re-runs the body. But the caller walk is not a query; it is called from inside those bodies, once per caller-side argument read plus once for each caller-walk fallback. So a single evaluation can enter it several times, rebuilding the same caller's reachability each time.

Why It Is Probably Not Urgent

The map is keyed on the caller and filled from getPredNodes, and most callees here have very few predecessors. An instrumented whole-project run of a consumer reported 422 queries over its dependency graph, which is the right order of magnitude for the multiplier rather than the thousands that would make CFG-sized work alarming.

Stated at its real strength: that is read from the code and inferred from someone else's query count. Nobody has profiled it. The cheap measurement is a counter on entry to the caller walk and another on the reachability computation, whose ratio answers it directly.

The Correctness Question Attached To The Fix

Hoisting the map to a per-solve cache keyed on the caller looks obviously sound, because nothing in it depends on the query and the reachability computation takes an empty environment, making it a function of the caller alone.

It is sound only if the branch fold's own state cannot change WITHIN a solve. The recently repaired phi handling in resolveComparableConstant is exactly the machinery that decides whether a branch prunes, so a cache outliving a change in what folds would serve stale reachability, and the symptom would be reachability disagreeing with itself rather than anything failing loudly.

That check belongs with the optimisation rather than after it. It is recorded here because the optimisation is the kind someone reaches for while looking at something else, and the question does not announce itself.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions