Message system: group block repeats by name and pool the per-message data graph#305
Merged
Conversation
LLMsgData kept each decoded/built block in a std::map keyed by the block name's string-table pointer plus the repeat index (name + blocknum). String-table names are contiguous 64-byte rows, so name + i for i past ~64 walked into the adjacent name's storage; two block names within range plus enough repeats aliased keys, overwriting (and leaking) blocks and corrupting data. ImprovedTerseObjectUpdate-class messages reach ~180 repeats after zerocode expansion, so this was reachable from inbound traffic -- the long-standing "should really change to a vector" TODO. Give each block name an ordered vector of its repeat instances, with the groups themselves held in first-seen (template) order. getBlock(name, i) recovers repeat i by position, so no pointer outside a real object is ever formed and the per-repeat map node is gone. buildBlock() indexes repeats directly instead of walking ++map_iter and assuming the synthesized keys sort adjacently; both copyFromMessageData() paths iterate groups x repeats. The builder still tracks its instance count on the group header (mBlockNumber) to preserve the declared-but-empty placeholder semantics newMessage()/buildBlock() rely on. LLMsgBlkData stays heap-allocated and pointer-stable so the cached mCurrentSDataBlock / cur_data_block / mbci pointers can't dangle across a vector growth; pooling those objects is a separate follow-up. Drop dead LLMsgData::addDataFast (no callers, carried its own copy of the synthesized-pointer logic). Reader getSize() now uses the find-based const getBlock() template overload so a missing block can't insert into the template. Add a regression test: a variable block repeated 200 times alongside a second block, round-tripped, verifying every repeat and the neighbor are intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Rye <rye@alchemyviewer.org>
The template reader freed and reallocated its whole working data set every packet -- new LLMsgData plus a new LLMsgBlkData (each eagerly reserving a variable vector) per block repeat, all torn down again at clearMessage(). Under object-update load that is the hottest allocation path in the viewer (ImprovedTerseObjectUpdate-class messages carry ~180 repeats). Keep the reader's LLMsgData alive across packets and reset it in place instead. reset() recycles every block into an internal free pool and clears the groups while keeping all vector capacity; allocBlock() hands blocks back out from the pool (reinit clears the prior variable data, freeing any heap payloads). After warmup a steady-state decode performs no heap allocation. clearMessage() now recycles rather than deletes; every getter already gates on mReceiveSize == -1, so the reset-but-alive object is never read stale. LLMsgBlkData stays heap-allocated and pointer-stable, so cached mCurrentRMessageData / cur_data_block pointers within a decode are unaffected. Add a reuse-path test: alternate a 5-repeat and a 2-repeat message through one reader, verifying counts and values each pass so no stale block or value leaks across the differing repeat counts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Rye <rye@alchemyviewer.org>
LLTemplateMessageBuilder reallocated its working data set on every newMessage() -- a fresh LLMsgData plus one LLMsgBlkData per template block, freed again at clearMessage(). AgentUpdate and friends go out every frame, so this mirrors the inbound decode allocation churn. Reuse the builder's LLMsgData the same way the reader now does: reset it in place in newMessage() (and clearMessage()) instead of delete+new, and draw block instances from its pool via allocBlock(). Use is gated by mCurrentSMessageTemplate, which clearMessage() nulls, so the reset-but-alive object is never touched between messages. Add a test that drives one builder through alternating 5- and 2-block messages, round-tripping each, so a reused build can't inherit blocks or values from the previous one. Bump the group's max-tests headroom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Rye <rye@alchemyviewer.org>
An empty MVT_VARIABLE field stores no payload, and since the inline- storage change getData() returns null for it, so copyFromMessageData's unconditional read of the last byte (checking for a terminating NUL) became a null[-1] access; addBinaryData's &v[0] on a size-0 vector was likewise out of contract. Guard both on size, and have buildBlock's MBT_MULTIPLE underfill error name the block from the template instead of dereferencing a block-data pointer that can be null there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 11 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR refactors message template block storage from a map-based design to a grouping-and-pooling model with recycled block allocation. ChangesMessage Template Pooling and Grouping
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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
🤖 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 `@indra/llmessage/lltemplatemessagebuilder.cpp`:
- Around line 760-776: The current copyFromMessageData iterates every block and
every var, which copies template scaffolding; instead locate the header block
for each group (the first LLMsgBlkData in LLMsgData::BlockGroup mBlocks) read
its repeat count and drive nextBlock(...) that many times, and when copying vars
skip any LLMsgVarData with getSize() < 0 so placeholder vars are not copied;
update references in copyFromMessageData to use the header block's repeat count
(e.g., headerBlock->mRepeatCount or headerBlock->getRepeatCount()) and only call
addData(...) for vars whose getSize() >= 0.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e1d75459-1e4f-4c31-96fd-e7b6cdc3c01b
📒 Files selected for processing (7)
indra/llmessage/llmessagetemplate.cppindra/llmessage/llmessagetemplate.hindra/llmessage/llsdmessagebuilder.cppindra/llmessage/lltemplatemessagebuilder.cppindra/llmessage/lltemplatemessagereader.cppindra/llmessage/tests/llsdmessagebuilder_test.cppindra/llmessage/tests/lltemplatemessagebuilder_test.cpp
💤 Files with no reviewable changes (1)
- indra/llmessage/llmessagetemplate.cpp
Builder-owned LLMsgData carries a size -1 placeholder for each variable of an opened block until addData() fills it, and a placeholder has no payload at all, so both copyFromMessageData() implementations would read through a null data pointer if handed such data. No production path does (every copyToBuilder source is reader-decoded data, which fills every variable), but the guard is one branch. Block-level walking is unchanged on purpose: pre-added header blocks were emitted by the old map walk too, and the hand-built data convention in the LLSD builder tests expects blocks with a zero repeat count to be copied. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Restructures
LLMsgData, the per-message block/variable data model shared by the template message reader and builder, fixing a reachable data-corruption defect and removing all steady-state heap allocation from both the UDP decode and send paths.Block repeats grouped by name (corruption fix)
The old model keyed each block repeat in a
std::map<char*, LLMsgBlkData*>byname + repeat_indexpointer arithmetic over the prehashed string table. String-table names are contiguous 64-byte rows, so past ~64 repeats the synthesized keys walked into the adjacent name''s storage; with two block names in range, keys aliased — overwriting (and leaking) blocks and corrupting decoded data. ImprovedTerseObjectUpdate-class messages reach ~180 repeats after zerocode expansion, so this was reachable from normal inbound traffic.Each block name now owns an ordered vector of its repeat instances (
BlockGroup), kept in template order. No synthesized keys, no per-block map nodes, andgetBlock(name, i)recovers repeat i directly. As a side effect the builder''sMBT_MULTIPLEoverflow check now fires at exactly the limit (the old check read the previous repeat''s index and was off by one — and checked the wrong block entirely when calls interleaved block names); overpacking was already fatal at build time, so nothing could have depended on the laxness.Pooled, reusable data graph (decode + send)
LLMsgDatanow recycles its blocks through an internal free pool:reset()returns every block to the pool keeping all vector capacity, andallocBlock()reuses them. The reader and builder keep theirLLMsgDataalive across messages instead of delete+new per packet, so after warmup a steady-state decode (object updates every frame) and send (AgentUpdate every frame) allocate nothing. Stale-state access is gated as before: every reader getter bails onmReceiveSize == -1and every builder entry point on a nullmCurrentSMessageTemplate.Empty-variable guards in the LLSD copy path
LLSDMessageBuilder::copyFromMessageDataunconditionally read the last payload byte ofMVT_VARIABLEfields to detect NUL termination; for an empty field (no payload,getData()null since the inline-storage change) that was a null[-1]access, andaddBinaryData''s&v[0]on a size-0 vector was likewise out of contract. Both are now guarded on size, andbuildBlock''s MULTIPLE-underfill error names the block from the template rather than dereferencing a possibly-null block pointer.Tests
New TUT coverage in
lltemplatemessagebuilder_testandllsdmessagebuilder_test:MVT_VARIABLEfield copies to LLSD as an empty value instead of reading out of boundsAll green in RelWithDebInfo and Debug: lltemplatemessagebuilder 50/50, llsdmessagebuilder 47/47, llsdmessagereader 21/21, message 1/1.
🤖 Generated with Claude Code