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

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see fault.

ref->getCommand()->copyBytesForNetPacket(bigPacketData, *ref);

NetCommandList* commandList = newInstance(NetCommandList);
commandList->init();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this call necessary?

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