Skip to content

refactor(network): Remove dynamic allocations for NetPacket#2866

Open
Caball009 wants to merge 2 commits into
TheSuperHackers:mainfrom
Caball009:remove_network_netpacket
Open

refactor(network): Remove dynamic allocations for NetPacket#2866
Caball009 wants to merge 2 commits into
TheSuperHackers:mainfrom
Caball009:remove_network_netpacket

Conversation

@Caball009

@Caball009 Caball009 commented Jul 9, 2026

Copy link
Copy Markdown

This PR makes two changes to remove all dynamic allocations for NetPacket:

  1. Refactors NetPacket::ConstructBigCommandList so that NetPacket doesn't have to be memory pooled.
  2. Removes memory pool for NetPacket, so that it can be stack allocated.

TODO:

  • Verify everything works ok.

@Caball009 Caball009 added Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing labels Jul 9, 2026
@greptile-apps

greptile-apps Bot commented Jul 9, 2026

Copy link
Copy Markdown

Greptile Summary

This PR removes all dynamic (pool/heap) allocations for NetPacket by eliminating its MemoryPoolObject base class and replacing ConstructBigCommandPacketList (which returned a list of heap-allocated packets) with ConstructBigCommandList (which returns a single NetCommandList*). Call sites in Connection.cpp and ConnectionManager.cpp now use stack-allocated NetPacket instances.

  • ConstructBigCommandList: The new implementation builds chunk refs and adds them directly to a NetCommandList via the addMessage(NetCommandRef*&) overload, which takes ownership — no leak. As a side effect it fixes a relay self-assignment bug in the old code where the inner ref variable shadowed the function parameter, leaving every chunk's relay at its default value of 0.
  • sendNetCommandMsg / doSend / doRelay: Stack-allocated NetPacket replaces the static singleton (in sendNetCommandMsg) and per-iteration heap allocations (in doSend and doRelay). MAX_PACKET_SIZE tops out at 1100 bytes so stack usage is well within safe limits.
  • Pool entries: Both GameMemoryInitPools_Generals.inl and GameMemoryInitPools_GeneralsMD.inl remove the NetPacket pool row, consistent with the class no longer being pool-managed.

Confidence Score: 5/5

Safe to merge once the author completes their own functional verification (noted in the PR TODO).

The refactoring is mechanically sound. Ownership is correctly transferred to NetCommandList via the addMessage(NetCommandRef*&) overload, ref-counting through wrapperMsg is preserved, and MAX_PACKET_SIZE (≤1100 bytes) keeps stack usage well within normal limits. The change also incidentally fixes the old relay self-assignment bug. No regressions were found in the changed paths.

No files require special attention beyond the author completing their functional test pass.

Important Files Changed

Filename Overview
Core/GameEngine/Include/GameNetwork/NetPacket.h Removes MemoryPoolObject base class and pool glue macros; changes constructor to take TransportMessage by const-ref; replaces NetPacketList/ConstructBigCommandPacketList with new ConstructBigCommandList returning NetCommandList*.
Core/GameEngine/Source/GameNetwork/NetPacket.cpp Rewrites ConstructBigCommandPacketList as ConstructBigCommandList, allocating chunks directly into a NetCommandList. Also fixes a relay self-assignment bug from the old code. Ownership model is correct: addMessage(NetCommandRef*&) takes ownership of each chunkRef.
Core/GameEngine/Source/GameNetwork/Connection.cpp Replaces heap-allocated NetPacket (static singleton in sendNetCommandMsg and per-iteration allocation in doSend) with stack-allocated instances. Simplifies the big-command path to iterate directly over NetCommandList instead of NetPacketList.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp Switches doRelay to use stack-allocated NetPacket per loop iteration; correctly removes the outer-loop deleteInstance(packet) which was a nullptr no-op in the original.
Core/GameEngine/Source/Common/System/GameMemoryInitPools_Generals.inl Removes the NetPacket pool entry, consistent with the class no longer deriving from MemoryPoolObject.
Core/GameEngine/Source/Common/System/GameMemoryInitPools_GeneralsMD.inl Same NetPacket pool entry removal as the Generals variant; symmetric change.

Reviews (5): Last reviewed commit: "Removed memory pool for NetPacket." | Re-trigger Greptile

@xezon

xezon commented Jul 11, 2026

Copy link
Copy Markdown

What is the motivation for removing memory pool? The main purpose of memory pool is cache and defrag friendlyness.

@Caball009

Copy link
Copy Markdown
Author

What is the motivation for removing memory pool? The main purpose of memory pool is cache and defrag friendlyness.

The purpose of a memory pool is to mitigate the downsides of frequent dynamic allocations. It's unnecessary if you don't need dynamic allocations.

@xezon

xezon commented Jul 11, 2026

Copy link
Copy Markdown

But it is still allocated with NetPacket *packet = new(NetPacket);

@Caball009 Caball009 force-pushed the remove_network_netpacket branch 3 times, most recently from 9b43034 to e1fead4 Compare July 13, 2026 13:12
Comment thread Core/GameEngine/Source/GameNetwork/NetPacket.cpp
@Caball009 Caball009 changed the title refactor(network): Remove memory pool for NetPacket refactor(network): Remove dynamic allocations for NetPacket Jul 13, 2026
@Caball009

Copy link
Copy Markdown
Author

But it is still allocated with NetPacket *packet = new(NetPacket);

Rewritten so that this is no longer the case.

@Caball009

Copy link
Copy Markdown
Author

@greptileai re-review this pull request.

@Caball009 Caball009 force-pushed the remove_network_netpacket branch from e1fead4 to 757d5cf Compare July 13, 2026 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Minor Severity: Minor < Major < Critical < Blocker Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants