Grow StateArena across owner batches#69
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes shared arena placement/alignment math into a small scene-detail header and updates StateArena so owner-side state allocation can grow across multiple batches (instead of being effectively one-shot per owner), with a new contract test covering the multi-batch behavior.
Changes:
- Added
common/app/scene/detail/ArenaMath.hppand movedAlignOf/NormalizeArenaAlignusage toloka::app::scene::detail::*. - Refactored
StateArenato use chained blocks and allowallocate()to append new blocks when the current one is exhausted. - Added a focused boundary/arena test that pins multi-batch owner locality and immediate state creation behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/BoundaryArenaTests.cpp | Adds a test owner + new contract test to verify StateArena grows across multiple owner batches and supports immediate state creation. |
| common/app/scene/state/StateBatchBase.hpp | Removes dependency on Node.hpp by using the new arena math header and detail:: alignment helpers. |
| common/app/scene/node/Conditional.cpp | Updates node alignment query to use detail::AlignOf. |
| common/app/scene/Node.hpp | Removes embedded arena math and includes detail/ArenaMath.hpp; updates node alignment to detail::AlignOf. |
| common/app/scene/detail/ArenaMath.hpp | New shared header providing AlignOf and NormalizeArenaAlign for arena sizing/allocation. |
| common/app/scene/composition/NodeComposition.cpp | Switches to detail/ArenaMath.hpp and detail::NormalizeArenaAlign for node arena sizing. |
| common/app/scene/boundary/detail/BoundaryArena.hpp | Implements growable StateArena via chained blocks; updates arena alignment normalization calls. |
| common/app/scene/boundary/Boundary.hpp | Always forwards reserve hints to StateArena (no longer gated on first-capacity). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3b79a347d
ℹ️ 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".
Summary
This PR handles two related owner-side arena cleanups:
AlignOf,NormalizeArenaAlign) intocommon/app/scene/detail/ArenaMath.hppStateArena::reserve()append owner-lifetime blocks across multiple explicit state batchesWhy
#56gives shared arena arithmetic one small scene-detail home instead of making state code depend onNode.hpp.#55fixes the owner-level locality hole left after#29: laterdeclareStates()batches can now reserve another arena block instead of falling back to heap merely because an earlier batch seeded the arena.The growth boundary remains explicit:
reserve()is the only operation that appends arena blocksallocate()consumes existing reserved capacity onlyValidation
cmake --build build/Testingcmake --build build/Testing-ASanctest --test-dir build/Testing --output-on-failure -R LokaFlowDslTestsctest --test-dir build/Testing-ASan --output-on-failure -R LokaFlowDslTestsRed evidence for the review regression: before
e9b6621, the new contract test failed becauseStateArena::allocate(1024, 4)appended a block after onlyreserve(32). After the fix, allocation returns0until an explicit reserve, and the owner path falls back to heap.Notes
#56:c0ec03e#55implementation:e3b79a3e9b6621std::nothrowunder the no-exception policyCloses #55
Closes #56