Skip to content

Commit 4b80f5f

Browse files
sharkdpdcreager
andauthored
[ty] Optimize TDD atom ordering (#20098)
## Summary While looking at some logging output that I added to `ReachabilityConstraintBuilder::add_and_constraint` in order to debug astral-sh/ty#1091, I noticed that it seemed to suggest that the TDD was built in an imbalanced way for code like the following, where we have a sequence of non-nested `if` conditions: ```py def f(t1, t2, t3, t4, …): x = 0 if t1: x = 1 if t2: x = 2 if t3: x = 3 if t4: x = 4 … ``` To understand this a bit better, I added some code to the `ReachabilityConstraintBuilder` to render the resulting TDD. On `main`, we get a tree that looks like the following, where you can see a pattern of N sub-trees that grow linearly with N (number of `if` statements). This results in an overall tree structure that has N² nodes (see graph below): <img alt="normal order" src="https://github.com/user-attachments/assets/aab40ce9-e82a-4fcd-823a-811f05f15f66" /> If we zoom in to one of these subgraphs, we can see what the problem is. When we add new constraints that represent combinations like `t1 AND ~t2 AND ~t3 AND t4 AND …`, they start with the evaluation of "early" conditions (`t1`, `t2`, …). This means that we have to create new subgraphs for each new `if` condition because there is little sharing with the previous structure. We evaluate the Boolean condition in a right-associative way: `t1 AND (~t2 AND (~t3 AND t4)))`: <img width="500" align="center" src="https://github.com/user-attachments/assets/31ea7182-9e00-4975-83df-d980464f545d" /> If we change the ordering of TDD atoms, we can change that to a left-associative evaluation: `(((t1 AND ~t2) AND ~t3) AND t4) …`. This means that we can re-use previous subgraphs `(t1 AND ~t2)`, which results in a much more compact graph structure overall (note how "late" conditions are now at the top, and "early" conditions are further down in the graph): <img alt="reverse order" src="https://github.com/user-attachments/assets/96a6b7c1-3d35-4192-a917-0b2d24c6b144" /> If we count the number of TDD nodes for a growing number if `if` statements, we can see that this change results in a slower growth. It's worth noting that the growth is still superlinear, though: <img width="800" height="600" alt="plot" src="https://github.com/user-attachments/assets/22e8394f-e74e-4a9e-9687-0d41f94f2303" /> On the actual code from the referenced ticket (the `t_main.py` file reduced to its main function, with the main function limited to 2000 lines instead of 11000 to allow the version on `main` to run to completion), the effect is much more dramatic. Instead of 26 million TDD nodes (`main`), we now only create 250 thousand (this branch), which is slightly less than 1%. The change in this PR allows us to build the semantic index and type-check the problematic `t_main.py` file in astral-sh/ty#1091 in 9 seconds. This is still not great, but an obvious improvement compared to running out of memory after *minutes* of execution. An open question remains whether this change is beneficial for all kinds of code patterns, or just this linear sequence of `if` statements. It does not seem unreasonable to think that referring to "earlier" conditions is generally a good idea, but I learned from Doug that it's generally not possible to find a TDD-construction heuristic that is non-pathological for all kinds of inputs. Fortunately, it seems like this change here results in performance improvements across *all of our benchmarks*, which should increase the confidence in this change: | Benchmark | Improvement | |---------------------|-------------------------| | hydra-zen | +13% | | DateType | +5% | | sympy (walltime) | +4% | | attrs | +4% | | pydantic (walltime) | +2% | | pandas (walltime) | +2% | | altair (walltime) | +2% | | static-frame | +2% | | anyio | +1% | | freqtrade | +1% | | colour-science | +1% | | tanjun | +1% | closes astral-sh/ty#1091 --------- Co-authored-by: Douglas Creager <[email protected]>
1 parent 5663426 commit 4b80f5f

File tree

1 file changed

+26
-4
lines changed

1 file changed

+26
-4
lines changed

crates/ty_python_semantic/src/semantic_index/reachability_constraints.rs

Lines changed: 26 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -424,9 +424,26 @@ impl ReachabilityConstraintsBuilder {
424424
}
425425
}
426426

427-
/// Returns whether `a` or `b` has a "larger" atom. TDDs are ordered such that interior nodes
428-
/// can only have edges to "larger" nodes. Terminals are considered to have a larger atom than
429-
/// any internal node, since they are leaf nodes.
427+
/// Implements the ordering that determines which level a TDD node appears at.
428+
///
429+
/// Each interior node checks the value of a single variable (for us, a `Predicate`).
430+
/// TDDs are ordered such that every path from the root of the graph to the leaves must
431+
/// check each variable at most once, and must check each variable in the same order.
432+
///
433+
/// We can choose any ordering that we want, as long as it's consistent — with the
434+
/// caveat that terminal nodes must always be last in the ordering, since they are the
435+
/// leaf nodes of the graph.
436+
///
437+
/// We currently compare interior nodes by looking at the Salsa IDs of each variable's
438+
/// `Predicate`, since this is already available and easy to compare. We also _reverse_
439+
/// the comparison of those Salsa IDs. The Salsa IDs are assigned roughly sequentially
440+
/// while traversing the source code. Reversing the comparison means `Predicate`s that
441+
/// appear later in the source will tend to be placed "higher" (closer to the root) in
442+
/// the TDD graph. We have found empirically that this leads to smaller TDD graphs [1],
443+
/// since there are often repeated combinations of `Predicate`s from earlier in the
444+
/// file.
445+
///
446+
/// [1]: https://github.com/astral-sh/ruff/pull/20098
430447
fn cmp_atoms(
431448
&self,
432449
a: ScopedReachabilityConstraintId,
@@ -439,7 +456,12 @@ impl ReachabilityConstraintsBuilder {
439456
} else if b.is_terminal() {
440457
Ordering::Less
441458
} else {
442-
self.interiors[a].atom.cmp(&self.interiors[b].atom)
459+
// See https://github.com/astral-sh/ruff/pull/20098 for an explanation of why this
460+
// ordering is reversed.
461+
self.interiors[a]
462+
.atom
463+
.cmp(&self.interiors[b].atom)
464+
.reverse()
443465
}
444466
}
445467

0 commit comments

Comments
 (0)