Repository navigation
Collapse inner loops with union-find during WTO construction - #9218
Conversation
During reverse-RPO natural loop discovery in WeakTopologicalOrdering, collapse each discovered loop body into its header using union-find with path compression, and skip over already-collapsed inner loops when walking immediate dominators in dominates(). This prevents outer loops from re-traversing inner loop bodies, bounding natural loop discovery to O(E alpha(N)) instead of O(N * depth) on deeply nested loops. Benchmark results across 16 WebAssembly modules (3 iterations, interleaved): - --constraint-analysis: - Geomean: 1.576s -> 1.564s (-0.7%) - Total time: 62.82s -> 62.40s (-0.7%) - --rse: - Geomean: 0.859s -> 0.853s (-0.7%) - Total time: 25.40s -> 25.06s (-1.3%)
# Conflicts: # src/cfg/wto.h
| (loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop) | ||
| (loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop) | ||
| (loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop) | ||
| (loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop)(loop) |
There was a problem hiding this comment.
If this PR speeds up this kind of mega-loop that suggests we care about such things. But didn't we say we don't care about mega-loops when we said recursion was fine?
There was a problem hiding this comment.
I'd say there are two kinds of speedup we care about: 1) wall clock speedup on real-world code, and 2) algorithmic complexity improvements to guard against pathological cases, which may occur rarely in real code or in e.g. fuzzer-generated code.
This test demonstrates the algorithmic complexity improvement. It takes over 5 seconds to run before this change.
There was a problem hiding this comment.
I lean towards this not mattering, though. 5 seconds isn't that bad, and this is extremely unrealistic code?
Adding a little complexity for things like this can lead to a lot of accumulated complexity for no practical benefit, I worry.
There was a problem hiding this comment.
This is a dramatic improvement in the time complexity of the algorithm, though. Wasn't that one of your concerns going into this?
There was a problem hiding this comment.
Yes, but to the extent it is realistic? Hundreds of nested loops is not realistic, and 5 seconds is really not that bad for such an unrealistic case.
But do you feel strongly here? I lean against it, but I don't want to block something you consider worthwhile.
There was a problem hiding this comment.
I'm getting more robust benchmarks for just this change to inform a decision.
There was a problem hiding this comment.
Ok, the performance differences due to this change are largely in the noise, except for these statistically significant runtime improvements:
- clang.wasm --rse: -8.6%
- KotlinConfApp-pre-wasm-opt.wasm --constraint-analysis: -5.2%
- esbuild.wasm --rse: -1.2%
The clang improvement in particular makes me think that this is worth landing, even setting aside the algorithmic improvements.
There was a problem hiding this comment.
Fair enough, happy to review and move forward
| // The index of the loop header we last traversed this node for, used | ||
| // instead of a `visited` set during the DFS. | ||
| Index lastVisitedBy = NoIndex; | ||
| // The parent in the union-find forest used to collapse inner loops into |
There was a problem hiding this comment.
Does union-find mean disjoint sets? If so, where is that data structure, since I don't seem to see this PR add one? And what is a "forest" of such things?
I feel like there is a bunch of concepts here without a large explanatory comment, or have I missed the forest for the trees 😆
There was a problem hiding this comment.
Yes, "union-find forest" and "disjoint sets" are two names for the same abstract data structure and algorithm. We have support/disjoint_sets.h, but we don't use it here because it is simpler and more efficient to just rearrange the existing references between entries into the union-find structure.
I can add a comment redirecting the reader to support/disjoint_sets.h to read about the algorithm, if you think that would help? Although that just links to wikipedia, so maybe better to link there directly: https://en.wikipedia.org/wiki/Disjoint-set_data_structure.
There was a problem hiding this comment.
I am more looking for a comment saying where the disjoint-set implementation is here. This doesn't add new data structures, so seemingly it was here all along, and this PR just "reinterprets" something existing? I'm not sure where to look for that.
There was a problem hiding this comment.
Well the only structural element in the data structure is each item's parent pointer. An item's parent pointer is unset if the item is the representative element of its set, and otherwise there is a path of parent pointers leading to the representative element for the set. So this ufParent is the entire data structure.
There was a problem hiding this comment.
Thanks! This is what I was looking for in a comment.
There was a problem hiding this comment.
Cool, I'll update the code comment to say something like that.
|
Darn, I accidentally merged this into the previous PR instead of into main :( |
2 participants
During reverse-RPO natural loop discovery in WeakTopologicalOrdering,
collapse each discovered loop body into its header using union-find with
path compression, and skip over already-collapsed inner loops when
walking immediate dominators in dominates(). This prevents outer loops
from re-traversing inner loop bodies, bounding natural loop discovery to
O(E alpha(N)) instead of O(N * depth) on deeply nested loops.
Benchmark results across 16 WebAssembly modules (3 iterations,
interleaved):