perf: study contiguous order-book storage#119
Conversation
|
Warning Review limit reached
More reviews will be available in 45 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements M47: Contiguous Order-Book Storage, adding a new fixed-band direct-price-indexed storage backend to OrderBook alongside existing baseline, pooled-node, and intrusive-pooled modes. The implementation includes the ContiguousStore backend, dispatch wiring throughout OrderBook, comprehensive tests, benchmark integration, and documentation of the bounded-domain architecture study. ChangesM47 Contiguous Order-Book Storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dec326608c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P2 on PR #119: a contiguous reprice whose re-add remainder would rest out of band was refused inside OrderBook::modify after the engine had already emitted OrderModified, so the event stream could report a modify the book never applied. OrderBook::can_apply_modify now answers the refusal before the event - MatchingEngine::modify no-ops without emitting (mirroring how can_store_limit gates new_limit), the gateway rejects such modifies with structured StorageExhausted, and the store keeps the refusal as defense in depth so direct book callers also keep the original order resting. ContiguousStore moves to src/engine/contiguous_store.hpp: the extra helpers pushed order_book.cpp to the brain-class function-count threshold, and the M47 backend is a coherent unit of its own. Both files score CodeScene 9.68 and the change-set quality gates pass; the engine pre-gate also cleared the pre-existing new_market/modify code duplication finding in matching_engine.cpp.
Artifact-only commit: results/pool_backed_storage.txt regenerated from the clean source commit above so the recorded source digest matches the modify pre-gate fix (Dirty inputs: no). All four storage modes still produce identical event counts, resting totals, and last_seq on the benchmark flow.
|
@coderabbitai pause |
✅ Action performedReviews paused. |
|
@codex review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd2e36e1ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7c40fe7e3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/qsl/engine/order_book.hpp (1)
30-34:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winQualify the storage-equivalence claim.
Storage::Contiguousdoes change observable behavior for out-of-band GTC remainders and reprices, becausecan_store_limit(...)/can_apply_modify(...)can refuse them. The "never changes matching semantics" sentence should be narrowed to the declared in-band price-domain assumption.Suggested wording
- // is fixed at construction and never changes matching semantics or determinism; it only - // changes where resting-order state lives in memory. + // is fixed at construction. Within the declared contiguous price-domain assumption it does + // not change matching semantics or determinism; it only changes where resting-order state + // lives in memory.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/qsl/engine/order_book.hpp` around lines 30 - 34, The sentence claiming Storage::Contiguous "never changes matching semantics or determinism" is too broad; update the comment to qualify that Storage::Contiguous can change observable behavior for out-of-band GTC remainders and reprices because can_store_limit(...) and can_apply_modify(...) may refuse them, and narrow the assertion to say it preserves matching semantics and determinism only under the declared in-band price-domain assumption; reference Storage::Contiguous, can_store_limit, and can_apply_modify in the revised comment so reviewers can locate and verify the limitation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engine/order_book.cpp`:
- Around line 370-380: The IntrusiveStore::add_limit path may call rest(...)
after matching and currently ignores rest failures, so preflight whether a GTC
remainder can be rested before calling OrderBook::match_incoming; mirror the
ContiguousStore::add_limit approach: if OrderBook::should_rest_limit(result,
input.tif) could leave a remainder, first validate pool capacity/can-rest for
input.id/input.side/input.price/result.remainder (using the same check
ContiguousStore uses) and if that preflight fails, avoid calling
OrderBook::match_incoming (or abort/return the appropriate error/indicator) so
the remainder is never silently dropped; update IntrusiveStore::add_limit and
any IntrusivePooled-specific helpers to perform this preflight check and
propagate failures instead of ignoring rest(...) return values.
---
Outside diff comments:
In `@include/qsl/engine/order_book.hpp`:
- Around line 30-34: The sentence claiming Storage::Contiguous "never changes
matching semantics or determinism" is too broad; update the comment to qualify
that Storage::Contiguous can change observable behavior for out-of-band GTC
remainders and reprices because can_store_limit(...) and can_apply_modify(...)
may refuse them, and narrow the assertion to say it preserves matching semantics
and determinism only under the declared in-band price-domain assumption;
reference Storage::Contiguous, can_store_limit, and can_apply_modify in the
revised comment so reviewers can locate and verify the limitation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2f9bd1fe-898c-4a46-9ff6-8f1e74ed0bf3
📒 Files selected for processing (13)
PROGRESS.mddocs/architecture.mddocs/pool_backed_storage.mdinclude/qsl/engine/matching_engine.hppinclude/qsl/engine/order_book.hppresults/pool_backed_storage.txtsrc/engine/contiguous_store.hppsrc/engine/matching_engine.cppsrc/engine/order_book.cppsrc/gateway/order_gateway.cpptests/unit/test_matching_engine.cpptests/unit/test_order_book.cpptests/unit/test_risk_gateway.cpp
✅ Files skipped from review due to trivial changes (3)
- results/pool_backed_storage.txt
- docs/architecture.md
- PROGRESS.md
CodeRabbit flagged that IntrusiveStore::add_limit ignored rest() failure:
a direct OrderBook{Storage::IntrusivePooled} caller that partially crossed
a GTC order and then ran out of pool capacity dropped the remainder, even
though the public add_limit contract rests it. It now runs can_store_limit
before matching (same guard the engine/gateway already applied, so their
behavior is unchanged), mirroring the contiguous-store fix.
Also narrow the Storage doc comment so the matching-semantics-preserved
claim is scoped to each mode's declared domain, add <utility> to
contiguous_store.hpp (std::move), and drop the now-unused <bit> from
order_book.cpp. Adds a direct-book regression filling the intrusive pool
and asserting a no-capacity bid is refused with maker liquidity intact.
Moving ContiguousStore to its own header surfaced its 5-argument add_limit as a newly-introduced CodeScene finding in the new file. Hoist the LimitInput bundle (id/side/price/quantity/tif) that IntrusiveStore already used up to OrderBook scope and take it in both backends' add_limit, so each storage entry point is single-argument and the near-duplicate struct is removed. Behavior-preserving: the public OrderBook::add_limit keeps its flat signature and only constructs the bundle at the dispatch site.
Artifact-only: regenerate results/pool_backed_storage.txt from the clean tree so its source digest matches the post-review source inputs (Dirty inputs: no). All four storage modes still produce identical event counts, resting totals, and last_seq on the benchmark flow.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 169d61611c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex P2s on PR #119: - docs/pool_backed_storage.md said contiguous was the fastest row, but the committed artifact has PMR 209.6 < contiguous 222.4 < baseline 273.7 < intrusive 373.0 ns/cmd. Restate the actual ordering (PMR fastest, contiguous second, both ahead of baseline; intrusive slower than baseline) per the repo's measured-only benchmark discipline. - MILESTONES.md assigned PR #119 to M45, but #119 is M47 and M45 landed as PR #117. Mark M45 merged (#117), and likewise the stale M44 (#115) and M46 (#118), and set M47 as the active PR #119 so resume/finish workflows route correctly. Also repaired a fused PROGRESS.md decision-log line. Docs/roadmap only; no source or artifact numbers changed.
There was a problem hiding this comment.
(1 files improve in Code Health)
Our agent can fix these. Install it.
Gates Passed
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| matching_engine.cpp | 9.10 → 9.69 | Code Duplication |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@codex review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Milestone
M47 — Contiguous order-book storage and cache-locality study.
Summary
OrderBook::Storage::Contiguous, an opt-in fixed-band direct price-indexed storage mode.[1, 1024], occupancy bitmaps for best-level discovery, and contiguous per-level FIFO vectors.EngineSnapshot, andlast_seq.qsl-bench storage/make bench-storageand regeneratesresults/pool_backed_storage.txtwith source-digest provenance.Evidence
results/pool_backed_storage.txtsha256:ecdd996360bc4b95e6d9111feda3daacccc9c346cceb3d8e88a901c3e9be1dfbnoTests
./build/dev/tests/test_matching_engine— passed, 17 test cases / 65647 assertions./build/dev/tests/test_order_book— passed, 23 test cases / 126 assertionsgit diff --check— passedmake check— passed, 225/225 testsmake asan— passed, 225/225 testsmake bench-storage— passed and regenerated the artifactNotes
Summary by CodeRabbit
New Features
Behavior Changes
Documentation
Tests & Benchmarks