Reduce per-packet allocations in networking hot path#540
Merged
Conversation
PacketReader: Replace MemoryStream with direct byte[] + int position. Eliminates MemoryStream allocation per packet and removes temp byte[] allocations in ReadInt16/ReadInt32 (was 10-80 allocs per packet). Add bounds safety matching old MemoryStream behavior. Expand test coverage from 1 to 35 tests. IncomingPacketHandlers: Cache Version objects, tab separator, and XYComparer as static readonly fields. Call GetItems() once in OnMobileIncoming instead of 3x. Replace LINQ anonymous type chains in OnItemAddedToContainer and OnItemDeleted with simple for loops. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughThe PR refactors Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ClassicAssist/UO/Data/PacketReader.cs (1)
97-109:⚠️ Potential issue | 🟠 Major
ReadString()is off by one at EOF.Line 103 requires one spare byte before it will read anything. If the terminator sits in the final byte, it never gets consumed; if the string is unterminated, the last character is dropped outright.
Suggested fix
- while ( _position + 1 < _size && ( c = _data[_position++] ) != 0 ) + while ( _position < _size && ( c = _data[_position++] ) != 0 )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist/UO/Data/PacketReader.cs` around lines 97 - 109, ReadString currently uses while (_position + 1 < _size && (c = _data[_position++]) != 0) which skips the final byte and drops the last char for unterminated strings; change the loop to run while (_position < _size), read c = _data[_position++], break if c == 0, otherwise append (char)c to the StringBuilder so the terminator in the final byte is consumed and the last character of an unterminated string is preserved; update the ReadString method accordingly.
🧹 Nitpick comments (1)
ClassicAssist.Tests/PacketReaderTests.cs (1)
373-382: Please extend the boundary coverage aroundIndex.Line 381 only checks the returned string, so the EOF/null-terminator regression in
ReadString()still passes, and Lines 477-480 only exerciseReadByte(), so they won't catch truncatedReadInt16/ReadInt32/fixed-width reads driftingIndexpastSize. A couple ofIndexassertions here would lock those edges down.Also applies to: 471-480
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Tests/PacketReaderTests.cs` around lines 373 - 382, The test ReadStringEmptyNullTerminated validates only the returned string but not the PacketReader cursor, so regressions that mis-handle EOF/null-terminator and advance Index incorrectly will slip; update this test (and the similar block around the ReadByte tests at 471-480) to assert the PacketReader.Index (and optionally PacketReader.Size) after calling PacketReader.ReadString(), and add small additional tests that call ReadInt16/ReadInt32/fixed-width reads on truncated buffers to assert Index does not advance past Size (use the PacketReader constructor and methods ReadString, ReadByte, ReadInt16, ReadInt32 and check the Index property) to lock down boundary behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ClassicAssist/UO/Data/PacketReader.cs`:
- Around line 55-84: Multiple read methods (ReadByteArray, ReadInt16, ReadInt32
and other fixed-width readers) advance _position by the requested width even
when fewer bytes are available, which can leave _position > _size; change the
position update to clamp to Size by using the actual available count or
Math.Min(_position + requestedWidth, _size) so _position never exceeds _size
(e.g. replace "_position += length" with clamped advance for ReadByteArray, and
similarly clamp advances of 2 in ReadInt16 and 4 in ReadInt32 and in any other
fixed-width readers referenced in the comment).
- Around line 13-18: The PacketReader constructor does not validate its inputs
which can leave _data/_size/_position invalid; update the PacketReader(byte[]
data, int size, bool fixedSize) constructor to validate that data is not null
and that size is between 0 and data.Length (inclusive/appropriate), and ensure
the computed _position (fixedSize ? 1 : 3) is <= size; throw
ArgumentNullException for null data and ArgumentOutOfRangeException for invalid
size/position so GetData() and scalar read methods fail fast instead of later.
- Around line 236-251: The Seek method can set _position to a negative value (in
cases in SeekOrigin.Begin, Current, and End), which allows subsequent reads to
index _data with a negative index; modify Seek (the Seek(int offset, SeekOrigin
origin) method) to clamp or reject negative positions by ensuring _position is
never less than zero (e.g., after computing the new _position, if _position < 0
then set it to 0 or throw an ArgumentOutOfRangeException), and also ensure it
still enforces the existing upper bound against _size; update any callers or
comments if you choose to throw instead of clamping.
In `@ClassicAssist/UO/Network/IncomingPacketHandlers.cs`:
- Around line 1497-1531: The code clears the layer slot but leaves the item in
the owner's Equipment collection and also only searches Engine.Mobiles on
deletes (missing PlayerMobile), causing stale state; update the logic around
FindLayerBySerial / SetLayer (the branch handling non-mobile containerSerial and
the Mobile delete branch that uses Engine.Mobiles.SelectEntity and
mobile.SetLayer) to also remove the item from the owner's Equipment list (e.g.,
call the same removal used when unequipping) whenever you clear a layer, and
extend deletion handling to check/handle PlayerMobile (Engine.Player) in
addition to Engine.Mobiles so player-equipped items are cleaned up; ensure the
fix is applied to the other similar blocks mentioned (the other occurrences of
the same pattern) so layer and Equipment stay in sync.
---
Outside diff comments:
In `@ClassicAssist/UO/Data/PacketReader.cs`:
- Around line 97-109: ReadString currently uses while (_position + 1 < _size &&
(c = _data[_position++]) != 0) which skips the final byte and drops the last
char for unterminated strings; change the loop to run while (_position < _size),
read c = _data[_position++], break if c == 0, otherwise append (char)c to the
StringBuilder so the terminator in the final byte is consumed and the last
character of an unterminated string is preserved; update the ReadString method
accordingly.
---
Nitpick comments:
In `@ClassicAssist.Tests/PacketReaderTests.cs`:
- Around line 373-382: The test ReadStringEmptyNullTerminated validates only the
returned string but not the PacketReader cursor, so regressions that mis-handle
EOF/null-terminator and advance Index incorrectly will slip; update this test
(and the similar block around the ReadByte tests at 471-480) to assert the
PacketReader.Index (and optionally PacketReader.Size) after calling
PacketReader.ReadString(), and add small additional tests that call
ReadInt16/ReadInt32/fixed-width reads on truncated buffers to assert Index does
not advance past Size (use the PacketReader constructor and methods ReadString,
ReadByte, ReadInt16, ReadInt32 and check the Index property) to lock down
boundary behavior.
🪄 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: 0dbee2bf-10c5-445d-8e2c-0647dfd66be9
📒 Files selected for processing (3)
ClassicAssist.Tests/PacketReaderTests.csClassicAssist/UO/Data/PacketReader.csClassicAssist/UO/Network/IncomingPacketHandlers.cs
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.
PacketReader: Replace MemoryStream with direct byte[] + int position. Eliminates MemoryStream allocation per packet and removes temp byte[] allocations in ReadInt16/ReadInt32 (was 10-80 allocs per packet). Add bounds safety matching old MemoryStream behavior. Expand test coverage from 1 to 35 tests.
IncomingPacketHandlers: Cache Version objects, tab separator, and XYComparer as static readonly fields. Call GetItems() once in OnMobileIncoming instead of 3x. Replace LINQ anonymous type chains in OnItemAddedToContainer and OnItemDeleted with simple for loops.
Summary by CodeRabbit
Release Notes
Tests
Refactor