Reuse the traversal key path in TestFilter's precomputed filter#1744
Reuse the traversal key path in TestFilter's precomputed filter#1744inju2403 wants to merge 1 commit into
Conversation
TestFilter's precomputed filter receives each node's key path from the graph traversal but discards it and reconstructs the same value via `item.test.id.keyPathRepresentation` — re-allocating a [String] and re-formatting the source location to a string per node. Since the graph is built by inserting each test at `test.id.keyPathRepresentation`, the node's key path *is* that value, so use it directly. Simpler, and removes a per-node allocation + string format from the filtering path. (~3.7x on the isolated per-node membership check; absolute per-run cost is sub-ms — this is a readability cleanup first.)
7fa17ac to
c7cf6d2
Compare
|
Do you have a rough idea of what percentage of time this represents during a test plan execution? I would imagine 35ms over 10,000 test runs to be a rounding error compared to the tests themselves. Not saying we shouldn't take this change (it's a good change) but I'm wondering what the real-world, non-micro-benchmark impact of it is. |
|
@harlanhaskins |
Reuse the key path the graph traversal already provides instead of recomputing it from each test's ID while filtering.
Motivation
Following the key-path work in #1725 and #1730 — which made the
Graphtraversal thread its key path efficiently — this change makes a consumer of that traversal actually use it.When a precomputed (ID-based)
Configuration.TestFilteris applied,Operation.apply(to:)walks the test graph withmapValues, which hands each closure the node's key path. The.precomputedbranches discard it (mapValues { _, item in ... }) and instead callselection.contains(item.test), which reconstructsitem.test.idand itskeyPathRepresentationfor every node — allocating a fresh[String]and formatting the source location into a"file:line:column"string each time.That work is redundant: the graph is built by inserting each test at
test.id.keyPathRepresentation(Runner.Plan._constructStepGraph/Plan.init(steps:)), so a node's key path in the graph is that test's key-path representation. The value being recomputed is, by construction, already in hand.Every path that applies the
.precomputedoperation does so against that same graph — or, for tag filters, a key-path preservingmapValuesof it intoFilterItem— so the invariant holds for ID, tag, and combined filters alike; intermediate nodes with no test value are skipped by the existingguard let item.Modifications
Configuration.TestFilter.Operation.apply(to:), the.precomputedincluding/excluding branches now use thekeyPathprovided bymapValues(selection.contains(keyPath)) instead ofselection.contains(item.test).test.id.keyPathRepresentation, mirroring the existing explanatory comment on the sibling.functioncase.The result is identical by construction, so behavior is unchanged. This is a small simplification that also drops a per-node ID reconstruction,
[String]allocation, and source-location string format from the filtering path (which runs on everyswift test --filterand tools-driven ID selection). No public API changes.Measurements
Isolated micro-benchmark of the per-node membership check (faithful replica of
keyPathRepresentation+ the trie walk, built with-O, results stable across runs):Unlike #1730, this is a constant-factor win (the speedup stays ~4× rather than growing), since it removes a fixed per-node cost rather than reducing complexity — so absolute savings scale linearly with the number of tests filtered (sub-millisecond for typical suites, ~26 ms once per run at 10k tests). The benchmark is a conservative lower bound: the old path additionally rebuilds
test.id, which the replica omits.Existing
PlanTestsandTestFiltertests pass — selection/exclusion by ID, by tag (via the.function→.precomputedpath), and combined filters.Checklist