BE-428: HashQL: Simplify traversal tracking to path recording#8498
BE-428: HashQL: Simplify traversal tracking to path recording#8498
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
This pull request is too large for Augment to review. The PR exceeds the maximum size limit of 100000 tokens (approximately 400000 characters) for automated code review. Please consider breaking this PR into smaller, more focused changes. |
3d9cc9d to
885a006
Compare
7014842 to
e6439e0
Compare
e6439e0 to
2346ece
Compare
2346ece to
5e31131
Compare
|
Deployment failed with the following error: |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Unbounded premiums become impossible placements
- I changed unbounded transfer midpoint fallback from
ApproxCost::INFto finiteCost::MAX-equivalent and added a regression test to ensure unbounded path premiums stay finite.
- I changed unbounded transfer midpoint fallback from
- ✅ Fixed: insert_range trusts inconsistent domain size
- I added an explicit capacity assertion in
FiniteBitSet::insert_rangeto reject oversizeddomain_sizeinputs and added a panic regression test.
- I added an explicit capacity assertion in
Or push these changes by commenting:
@cursor push 361796ea03
Preview (361796ea03)
diff --git a/libs/@local/hashql/core/src/id/bit_vec/finite.rs b/libs/@local/hashql/core/src/id/bit_vec/finite.rs
--- a/libs/@local/hashql/core/src/id/bit_vec/finite.rs
+++ b/libs/@local/hashql/core/src/id/bit_vec/finite.rs
@@ -230,6 +230,8 @@
I: [const] Id,
T: [const] FiniteBitSetIntegral,
{
+ assert!(domain_size <= T::MAX_DOMAIN_SIZE as usize);
+
let Some((start, end)) = inclusive_start_end(bounds, domain_size) else {
return;
};
@@ -620,6 +622,13 @@
}
#[test]
+ #[should_panic(expected = "assertion failed")]
+ fn insert_range_domain_size_exceeds_capacity_panics() {
+ let mut set: FiniteBitSet<TestId, u8> = FiniteBitSet::new_empty(8);
+ set.insert_range(.., 9);
+ }
+
+ #[test]
fn union_combines_bits() {
let mut a: FiniteBitSet<TestId, u8> = FiniteBitSet::new_empty(8);
a.insert(TestId::from_usize(0));
diff --git a/libs/@local/hashql/mir/src/pass/execution/cost/analysis.rs b/libs/@local/hashql/mir/src/pass/execution/cost/analysis.rs
--- a/libs/@local/hashql/mir/src/pass/execution/cost/analysis.rs
+++ b/libs/@local/hashql/mir/src/pass/execution/cost/analysis.rs
@@ -1,6 +1,6 @@
use core::alloc::Allocator;
-use super::{ApproxCost, StatementCostVec};
+use super::{ApproxCost, Cost, StatementCostVec};
use crate::{
body::basic_block::{BasicBlock, BasicBlockId, BasicBlockSlice, BasicBlockVec},
pass::{
@@ -124,7 +124,7 @@
let load = range
.saturating_mul(config.target_multiplier[target].get())
.midpoint()
- .map_or(ApproxCost::INF, From::from);
+ .map_or_else(|| ApproxCost::from(Cost::MAX), ApproxCost::from);
BasicBlockTargetCost { base, load }
}
@@ -511,6 +511,45 @@
assert!(interpreter_cost > interpreter_base);
}
+ /// Unbounded transfer-size estimates should stay expensive but finite, so they remain
+ /// valid candidates for solver search.
+ #[test]
+ fn unbounded_path_premium_stays_finite() {
+ let heap = Heap::new();
+ let interner = Interner::new(&heap);
+ let env = Environment::new(&heap);
+
+ let body = body!(interner, env; [graph::read::filter]@0/2 -> ? {
+ decl env: (), vertex: [Opaque sym::path::Entity; ?], val: ?;
+ @proj properties = vertex.properties: ?;
+
+ bb0() {
+ val = load properties;
+ return val;
+ }
+ });
+
+ let targets = make_targets(&body, all_targets());
+ let targets = BasicBlockSlice::from_raw(&targets);
+
+ let costs: TargetArray<StatementCostVec<Global>> =
+ TargetArray::from_fn(|_| StatementCostVec::from_iter([1].into_iter(), Global));
+
+ let analysis = BasicBlockCostAnalysis {
+ vertex: VertexType::Entity,
+ assignments: targets,
+ costs: &costs,
+ };
+
+ let result = analysis.analyze_in(&default_config(), &body.basic_blocks, Global);
+ let bb0 = BasicBlockId::new(0);
+
+ let interpreter_cost = result.cost(bb0, TargetId::Interpreter);
+ let interpreter_base = costs[TargetId::Interpreter].sum_approx(bb0);
+ assert!(interpreter_cost > interpreter_base);
+ assert!(interpreter_cost.is_finite());
+ }
+
/// Paths across multiple blocks are analyzed independently per block.
#[test]
fn paths_across_blocks_independent() {This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
| } | ||
| .analyze_in( | ||
| &TransferCostConfig::new(InformationRange::full()), | ||
| &body.basic_blocks, |
There was a problem hiding this comment.
Unbounded premiums become impossible placements
High Severity
ExecutionAnalysis builds TransferCostConfig with InformationRange::full(), so unbounded paths like EntityPath::Properties produce None from midpoint() and are converted to ApproxCost::INF. Those infinite block costs can make valid cyclic assignments look infeasible during solver search, causing false unsatisfiable placement results.
Additional Locations (1)
| I: [const] Id, | ||
| T: [const] FiniteBitSetIntegral, | ||
| { | ||
| let Some((start, end)) = inclusive_start_end(bounds, domain_size) else { |
There was a problem hiding this comment.
insert_range trusts inconsistent domain size
Medium Severity
FiniteBitSet::insert_range now accepts an external domain_size but never validates it against T::MAX_DOMAIN_SIZE. If a larger domain is passed, end can exceed the backing width and the T::MAX_DOMAIN_SIZE - 1 - end math underflows, leading to panic or incorrect masking instead of a bounded range insert.
Benchmark results
|
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2002 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1001 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 3314 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 1526 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 2078 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 1033 | Flame Graph |
policy_resolution_medium
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 102 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 51 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 269 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 107 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 133 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 63 | Flame Graph |
policy_resolution_none
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 2 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 8 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 3 | Flame Graph |
policy_resolution_small
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| resolve_policies_for_actor | user: empty, selectivity: high, policies: 52 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: empty, selectivity: medium, policies: 25 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: high, policies: 94 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: seeded, selectivity: medium, policies: 26 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: high, policies: 66 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: low, policies: 1 | Flame Graph | |
| resolve_policies_for_actor | user: system, selectivity: medium, policies: 29 | Flame Graph |
read_scaling_complete
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id;one_depth | 1 entities | Flame Graph | |
| entity_by_id;one_depth | 10 entities | Flame Graph | |
| entity_by_id;one_depth | 25 entities | Flame Graph | |
| entity_by_id;one_depth | 5 entities | Flame Graph | |
| entity_by_id;one_depth | 50 entities | Flame Graph | |
| entity_by_id;two_depth | 1 entities | Flame Graph | |
| entity_by_id;two_depth | 10 entities | Flame Graph | |
| entity_by_id;two_depth | 25 entities | Flame Graph | |
| entity_by_id;two_depth | 5 entities | Flame Graph | |
| entity_by_id;two_depth | 50 entities | Flame Graph | |
| entity_by_id;zero_depth | 1 entities | Flame Graph | |
| entity_by_id;zero_depth | 10 entities | Flame Graph | |
| entity_by_id;zero_depth | 25 entities | Flame Graph | |
| entity_by_id;zero_depth | 5 entities | Flame Graph | |
| entity_by_id;zero_depth | 50 entities | Flame Graph |
read_scaling_linkless
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | 1 entities | Flame Graph | |
| entity_by_id | 10 entities | Flame Graph | |
| entity_by_id | 100 entities | Flame Graph | |
| entity_by_id | 1000 entities | Flame Graph | |
| entity_by_id | 10000 entities | Flame Graph |
representative_read_entity
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1
|
Flame Graph | |
| entity_by_id | entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1
|
Flame Graph |
representative_read_entity_type
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| get_entity_type_by_id | Account ID: bf5a9ef5-dc3b-43cf-a291-6210c0321eba
|
Flame Graph |
representative_read_multiple_entities
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| entity_by_property | traversal_paths=0 | 0 | |
| entity_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| entity_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=0 | 0 | |
| link_by_source_by_property | traversal_paths=255 | 1,resolve_depths=inherit:1;values:255;properties:255;links:127;link_dests:126;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:0;link_dests:0;type:false | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:0;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:0;properties:2;links:1;link_dests:0;type:true | |
| link_by_source_by_property | traversal_paths=2 | 1,resolve_depths=inherit:0;values:2;properties:2;links:1;link_dests:0;type:true |
scenarios
| Function | Value | Mean | Flame graphs |
|---|---|---|---|
| full_test | query-limited | Flame Graph | |
| full_test | query-unlimited | Flame Graph | |
| linked_queries | query-limited | Flame Graph | |
| linked_queries | query-unlimited | Flame Graph |



🌟 What is the purpose of this PR?
Replaces the traversal extraction transform pass (which rewrote MIR bodies by splicing load statements and creating new locals) with a pure analysis pass that records which entity paths each block accesses as bitsets. Introduces per-block path transfer cost charging so the placement solver can account for data locality when assigning blocks to backends.
🔍 What does this change?
traversal/analysis/): walks the body, resolves vertex projections viaEntityPath::resolve(), records accessed paths asTraversalPathBitSet. No MIR mutation.cost/analysis.rs): newBasicBlockCostAnalysiscombines per-statement costs with a path transfer premium. For each block, charges a transfer premium on targets that are not the natural origin for the accessed paths (e.g., Interpreter pays a premium forVectorsbecause Embedding is the origin). Premiums are charged once per block, not per statement.BasicBlockCostVec: the placement solver now operates on precomputed per-block costs.PlacementSolverContexttakesblocks: &BasicBlockCostVecinstead of separate statement costs and assignment arrays.BlockPartitionedVec<T>: generic offset-table-backed per-block storage, extracted fromStatementCostVecinternals.TraversalLattice: lattice structure carryingVertexTypefor dataflow analysis over path bitsets.EntityPath::origin(),EntityPath::estimate_size(): per-path origin backend and transfer size estimation.TransferCostConfig: variable-size parameters (properties, embeddings, provenance sizes, per-target cost multiplier).FiniteBitSet::negate()in hashql-core.pass/transform/traversal_extraction/(the MIR transform and its tests).Pre-Merge Checklist 🚀
🚢 Has this modified a publishable library?
This PR:
📜 Does this require a change to the docs?
The changes in this PR:
🕸️ Does this require a change to the Turbo Graph?
The changes in this PR:
Path transfer premiums use pure over-approximation: every non-origin backend pays the full premium with no discounting for co-location. This can over-penalize non-origin assignment when multiple blocks access the same path on the same backend (cross-block dedup is not implemented). This is intentional: separable costs are a prerequisite for the PBQP solver planned in BE-436.
The ideal cost model is non-separable: the first block on a backend pays the full data fetch cost, subsequent co-labeled blocks pay zero ("shared-fetch" / pay-once-per-group). This maps to submodular clique costs in the optimization literature. Two frameworks formalize this:
The multi-label extension via alpha-expansion (Boykov, Veksler, Zabih 2001) reduces k-label to a sequence of binary cooperative cuts (each solvable exactly), but only guarantees a local minimum. Our
TransMatrixis not metric, so no global approximation factor holds. For k=3 backends (growing to 6-7), the overhead of iterative cooperative cut calls on ~100-node graphs is not justified when PBQP R1/R2 reductions on the separable over-approximation give exact solutions in microseconds.Pure over-approximation does not distort the relative ranking between non-origin backends (the premium cancels in pairwise comparisons) but inflates the absolute cost of non-origin assignment, biasing toward origin backends. Intra-block dedup (implemented here) is the first mitigation; cross-block dedup remains an open problem for k > 2 on cyclic graphs.
:
🛡 What tests cover this?
BasicBlockCostAnalysiscovering: no vertex access, single origin/non-origin paths, multiple path accumulation, composite path expansion, restricted target domains, cross-block independenceTraversalAnalysisVisitorcovering path resolution from MIR projectionsEntityPathBitSetcovering composite swallowing, lattice operations, normalizationBasicBlockCostVec; newpath_premiums_influence_placementandprovenance_variants_produce_different_premiumsintegration testsmixed_postgres_embedding_interpretertest verifying three-backend splitting❓ How to test this?
cargo nextest run --package hashql-mircargo test --package hashql-mir --doc