BE-583, BE-584, BE-586: HashQL: Add HeapPool, improve spans, and propagate Critical severity through diagnostics#8836
Conversation
chore: add new dependency chore: format feat: error module feat: introduce hashql_eval interner chore: checkpoint feat: checkpoint feat: checkpoint chore: remove old value module feat: checkpoint feat: checkpoint feat: checkpoint feat: checkpoint feat: checkpoint chore: checkpoint feat: move entity query into its own modul fix: query request feat: checkpoint (it compiles!) feat: checkpoint feat: checkpoint feat: checkpoint fix: issue around cached thunking feat: covariance for opaque inners fix: cfgattr serde chore: remove graph module
PR SummaryMedium Risk Overview J-Expr lexer/parser diagnostics now default to
Diagnostics gains Reviewed by Cursor Bugbot for commit d854dc5. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8836 +/- ##
==========================================
+ Coverage 59.15% 59.22% +0.06%
==========================================
Files 1346 1346
Lines 130117 130200 +83
Branches 5883 5883
==========================================
+ Hits 76975 77115 +140
+ Misses 52237 52180 -57
Partials 905 905
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens HashQL’s allocator and diagnostics infrastructure by introducing pooled Heap allocators (HeapPool) alongside a rewritten ScratchPool implementation, and by making JExpr lexer/parser errors statically critical (Diagnostic<…, Critical>) so downstream callers don’t need runtime severity enforcement. It also refactors span ancestry traversal for better ergonomics/perf characteristics and adds new diagnostic/status utilities (including span-mapping and serde support).
Changes:
- Replaces the
bump_scope::BumpPool-backedScratchPoolwith aMutex<Vec<_>>pool, addsHeapPool, and adds bounded-capacity retention with guard-return-on-drop semantics. - Propagates
Criticalseverity through JExpr lexer/parser diagnostics and removes runtime “ensure critical” mapping in the entity query REST handler. - Refactors
SpanTable::ancestorsto yield a lazy, deduplicated iterator (including the span itself) and addsLocalSpanIdfor typed local span indices; adds span/category mapping helpers + serde support in diagnostics.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| libs/@local/hashql/syntax-jexpr/src/test.rs | Makes diagnostic rendering helper generic over severity kind. |
| libs/@local/hashql/syntax-jexpr/src/parser/string/error.rs | Switches string parser diagnostics to Critical (defaulted type alias + constructors). |
| libs/@local/hashql/syntax-jexpr/src/parser/state.rs | Adds #[inline] to Expected conversions. |
| libs/@local/hashql/syntax-jexpr/src/parser/object/visit.rs | Forces object visitor diagnostics to be Critical. |
| libs/@local/hashql/syntax-jexpr/src/parser/object/error.rs | Switches object diagnostics to Critical and updates constructors. |
| libs/@local/hashql/syntax-jexpr/src/parser/error.rs | Switches parser diagnostics to Critical and inlines category conversions. |
| libs/@local/hashql/syntax-jexpr/src/parser/complex/mod.rs | Forces complex parser verification diagnostics to be Critical. |
| libs/@local/hashql/syntax-jexpr/src/parser/array/visit.rs | Forces array visitor diagnostics to be Critical. |
| libs/@local/hashql/syntax-jexpr/src/parser/array/error.rs | Switches array diagnostics to Critical and updates constructors. |
| libs/@local/hashql/syntax-jexpr/src/lexer/syntax_kind_set.rs | Adds #[inline] to small impls. |
| libs/@local/hashql/syntax-jexpr/src/lexer/mod.rs | Updates lexer error type alias usage (LexerDiagnostic) and signatures. |
| libs/@local/hashql/syntax-jexpr/src/lexer/error.rs | Switches lexer diagnostics to Critical and updates constructors. |
| libs/@local/hashql/syntax-jexpr/src/error.rs | Generalizes ResultExt over severity kind and updates JExpr diagnostic alias default. |
| libs/@local/hashql/diagnostics/src/status.rs | Adds serde derives for Success/Failure, adds StatusExt::map_spans, and introduces IntoStatus. |
| libs/@local/hashql/diagnostics/src/source/span.rs | Adds #[inline] on SourceSpan accessors. |
| libs/@local/hashql/diagnostics/src/lib.rs | Re-exports IntoStatus. |
| libs/@local/hashql/diagnostics/src/issues.rs | Adds DiagnosticIssues::map_spans, From<Vec<Diagnostic>>, and serde impls. |
| libs/@local/hashql/core/src/span/table.rs | Refactors SpanTable::ancestors to a lazy iterator with DenseBitSet dedup; adjusts indexing for LocalSpanId. |
| libs/@local/hashql/core/src/span/mod.rs | Introduces LocalSpanId and updates SpanId::id() return type. |
| libs/@local/hashql/core/src/pretty/formatter.rs | Adds #[inline] to FormatterOptions::default(). |
| libs/@local/hashql/core/src/module/namespace.rs | Adds #[inline] to ModuleNamespace::registry(). |
| libs/@local/hashql/core/src/lib.rs | Enables nightly nonpoison_mutex/sync_nonpoison and reorganizes feature list. |
| libs/@local/hashql/core/src/id/bit_vec/mod.rs | Adds #[inline] to bitset domain_size() accessors. |
| libs/@local/hashql/core/src/heap/scratch.rs | Minor attribute ordering and adds #[inline] to Default. |
| libs/@local/hashql/core/src/heap/pool.rs | Reimplements ScratchPool using Mutex<Vec<_>>, adds HeapPool, bounded capacity, guard drop-return, and unit tests. |
| libs/@local/hashql/core/src/heap/mod.rs | Exports HeapPool/HeapPoolGuard and adds #[inline] to Heap::default(). |
| libs/@local/hashql/core/src/graph/algorithms/dominators/frontier.rs | Adds #[inline] to a small accessor. |
| libs/@local/hashql/core/src/collections/pool.rs | Adds #[inline] to VecPool::capacity(). |
| libs/@local/hashql/core/Cargo.toml | Drops the rpds std feature flag. |
| libs/@local/hashql/compiletest/src/pipeline.rs | Generalizes diagnostics before boxing in the parse stage. |
| libs/@local/graph/api/src/rest/entity_query_request.rs | Removes runtime severity mapping; constructs Failure directly from critical diagnostics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Merging this PR will improve performance by 14.9%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | bit_matrix/dense/iter_row[64] |
170 ns | 140.8 ns | +20.71% |
| ⚡ | bit_matrix/dense/iter_row[200] |
215 ns | 185.8 ns | +15.7% |
| ⚡ | linear |
7.6 µs | 6.7 µs | +14% |
| ⚡ | diamond |
12.3 µs | 10.9 µs | +12.79% |
| ⚡ | complex |
18.1 µs | 16.2 µs | +11.53% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing bm/be-583-hashql-diagnostics-docs-and-span-improvements (d854dc5) with main (bf6fe44)1
Footnotes
fc41426 to
f4fbb1d
Compare
a58069f to
e6657ac
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit e6657ac. Configure here.
|
The branch was not rebased, yet. |
e6657ac to
d854dc5
Compare
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?
This PR introduces a
HeapPooltype (mirroring the existingScratchPool) for pooling and reusingHeapallocators across threads, and rewrites the underlying pool implementation to use a plainMutex<Vec<_>>instead ofbump_scope::BumpPool. Both pool types now support optional bounded capacity via abounded(max_size)constructor, and guards return their allocator to the pool (after resetting it) on drop.Alongside the pool work, the JExpr parser and lexer diagnostics are tightened so that all error-producing functions return
Diagnostic<_, _, Critical>directly rather than the genericSeverity-parameterised form. This removes the runtime severity-checking workaround in the entity query REST handler and makes the type system enforce that parse errors are always critical. Theancestorsmethod onSpanTableis also changed from returning aVec<SpanId>to returning a lazy iterator that includes the queried span itself, uses aDenseBitSetfor deduplication, and skips unresolvable spans.Additional smaller additions include
map_spansonDiagnosticIssuesandStatusExt,From<Vec<Diagnostic>>and serde impls forDiagnosticIssues, serde derives forSuccessandFailure, a newIntoStatustrait for convertingResult<T, Diagnostic<C, S, Critical>>into aStatus, andLocalSpanIdas a typed newtype for the local portion of aSpanId.🔍 What does this change?
bump_scope::BumpPool-backedScratchPoolwith aMutex<Vec<Scratch>>-based implementation; adds an equivalentHeapPool/HeapPoolGuardpair for poolingHeapallocators.new()(unbounded) andbounded(max_size)constructors; guards reset and return their allocator on drop, dropping it instead when the pool is at capacity.HeapPoolandHeapPoolGuardfromhashql_core::heap.rpdsstdfeature flag (no longer needed).nonpoison_mutexandsync_nonpoisonnightly features inhashql_core.Critical::ERRORinstead ofSeverity::Error, and updates allDiagnostictype aliases to default their severity kind parameter toCritical.EntityQuery::parse_expr;Failure::newis now called directly on the already-critical diagnostic.Diagnostic::generalizecall in the compiletest pipeline beforeDiagnostic::boxed.SpanTable::ancestorsto return a lazyimpl IntoIterator<Item = SpanId>that yields the queried span first, usesDenseBitSetfor O(1) duplicate detection, and skips spans not present in the table.LocalSpanIdas a typed newtype (bounded0..=SpanId::MAX_ID) for the local index portion of aSpanId;SpanId::id()now returnsLocalSpanId.DiagnosticIssues::map_spans,StatusExt::map_spans,From<Vec<Diagnostic>>forDiagnosticIssues, and serdeSerialize/Deserializeimpls forDiagnosticIssues,Success, andFailure.IntoStatustrait and a blanket impl forResult<T, Diagnostic<C, S, Critical>>, exported fromhashql_diagnostics.Failure::newdoc example and#[inline]annotations on various small accessor methods.Send/Syncassertions for all four pool types.ScratchPoolandHeapPoolcovering reuse, concurrent demand, and bounded-capacity behaviour.🛡 What tests cover this?
hashql_core::heap::poolcoveringScratchPoolandHeapPoolreuse, concurrent borrowing, and bounded-capacity eviction.❓ How to test this?
cargo test -p hashql-coreand confirm the new pool tests pass.cargo test -p hashql-syntax-jexprand confirm all parser/lexer snapshot tests are unchanged.