perf(network): Reduce packet allocations and improve loop logic by Caball009 · Pull Request #2659 · TheSuperHackers/GeneralsGameCode

Caball009 · 2026-04-28T18:47:17Z

This PR makes modest performance improvements in the networking code by reducing the number of dynamic NetPacket allocations and breaking out loops as early as possible.

See commits for cleaner diffs.

TODO:

Check whether it still makes sense to have NetPacket memory pooled. -> This can be dealt with in a separate PR: refactor(network): Remove memory pool for NetPacket #2866

stephanmeesters · 2026-04-28T22:48:27Z

I think it would be nice if we can change the signature of NetPacket from class NetPacket : public MemoryPoolObject to struct NetPacket. The memory pooling right now doesn't really get used, especially with your change? By the looks of it this would mostly need changes around NetPacket::ConstructBigCommandPacketList

Caball009 · 2026-04-29T10:08:36Z

I think it would be nice if we can change the signature of NetPacket from class NetPacket : public MemoryPoolObject to struct NetPacket. The memory pooling right now doesn't really get used, especially with your change? By the looks of it this would mostly need changes around NetPacket::ConstructBigCommandPacketList

Perhaps I can just put the relevant logic from Connection::sendNetCommandMsg in a callback and pass that to NetPacket::ConstructBigCommandPacketList.

Caball009 · 2026-06-02T20:27:34Z

I think it would be nice if we can change the signature of NetPacket from class NetPacket : public MemoryPoolObject to struct NetPacket. The memory pooling right now doesn't really get used, especially with your change? By the looks of it this would mostly need changes around NetPacket::ConstructBigCommandPacketList

I think it's best to do this in a separate pull request because it involves another set of changes.

greptile-apps · 2026-07-09T19:06:26Z

Greptile Summary

This PR reduces dynamic NetPacket heap allocations across the networking layer by replacing per-call newInstance/deleteInstance pairs with function-scoped static instances (reset via reset() before each use), and accelerates inbound-buffer iteration by breaking out of consumption loops at the first empty slot instead of scanning all MAX_MESSAGES entries.

Static NetPacket reuse: Connection::sendNetCommandMsg, Connection::doSend, and ConnectionManager::doRelay each now hold a single static NetPacket* initialized once; reset() replaces per-iteration allocation and is verified safe for sequential single-threaded access across multiple Connection instances.
Early-exit break in buffer consumers: ConnectionManager::doRelay, LANAPI::update, and NAT::connectionUpdate all break on the first zero-length m_inBuffer entry, relying on the dense-packing invariant upheld by Transport::doRecv (which fills slots sequentially from index 0, resetting bufferIndex to 0 on each call).
CopyTransportMessage extraction: The constructor's transport-copy logic is factored into a dedicated method callable on an already-initialized packet, enabling the static-reuse pattern in doRelay without re-allocating.

Confidence Score: 5/5

The changes are safe to merge; the optimization assumptions hold throughout the networking layer.

All three optimization pillars — static packet reuse, early-exit loop breaks, and CopyTransportMessage extraction — are correctly implemented. The early-exit break relies on m_inBuffer being densely packed, and this invariant is soundly maintained: doRecv resets bufferIndex to 0 on each call and fills sequentially, while every consumer clears slots from index 0 upward before breaking. No data can be orphaned between calls. The reuse of tempref instead of a fresh origref in the oversized-command path of sendNetCommandMsg is safe because addCommand does not take ownership of or modify the ref on failure.

No files require special attention.

Important Files Changed

Filename	Overview
Core/GameEngine/Include/GameNetwork/NetPacket.h	Constructor signature changed from pointer to const-ref; new CopyTransportMessage() method declared — clean, non-breaking API improvement.
Core/GameEngine/Source/GameNetwork/NetPacket.cpp	CopyTransportMessage() extracted from constructor; init() still called first in the constructor before CopyTransportMessage, so field initialization order is preserved.
Core/GameEngine/Source/GameNetwork/Connection.cpp	Two per-call NetPacket allocations replaced with function-scoped statics. Reuse of tempref for the oversized-command path is safe because addCommand does not take ownership or modify the ref on failure.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Static NetPacket replaces per-iteration allocation in doRelay; break on empty inBuffer slot is safe given the dense-packing invariant maintained by Transport::doRecv. The spurious deleteInstance(nullptr) at the end of the original doRelay is correctly removed.
Core/GameEngine/Source/GameNetwork/Transport.cpp	bufferIndex persists across Read() iterations within a single doRecv() call, avoiding redundant scans from index 0 for each packet. Dense-packing invariant is maintained: bufferIndex resets to 0 each doRecv() invocation, and consumers always clear from slot 0 upward.
Core/GameEngine/Source/GameNetwork/LANAPI.cpp	Loop modernized to use size_t and ARRAY_SIZE; break on empty slot added while the !LANbuttonPushed guard is correctly preserved in the loop condition.
Core/GameEngine/Source/GameNetwork/NAT.cpp	Loop modernized to use size_t and ARRAY_SIZE; break on empty slot correctly mirrors the dense-packing assumption applied in the other consumers.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Caller
    participant Connection
    participant Transport
    participant ConnectionManager
    participant NetPacket

    Note over Connection: Static NetPacket allocated once
    Caller->>Connection: sendNetCommandMsg(msg, relay)
    Connection->>NetPacket: reset()
    Connection->>NetPacket: addCommand(tempref)
    alt fits in one packet
        NetPacket-->>Connection: true
        Connection->>Connection: addMessage to m_netCommandList
    else too large to fit
        NetPacket-->>Connection: false
        Connection->>NetPacket: ConstructBigCommandPacketList(tempref)
        Connection->>Transport: queueSend per split packet
    end

    Note over Connection: Static NetPacket reused each loop
    Caller->>Connection: doSend()
    loop while commands remain
        Connection->>NetPacket: reset() then addCommand inner loop
        Connection->>Transport: queueSend(data)
    end

    Transport->>Transport: doRecv - fills m_inBuffer densely from index 0
    Note over Transport: bufferIndex persists within each doRecv call

    Caller->>ConnectionManager: doRelay()
    Note over ConnectionManager: Static NetPacket allocated once
    loop "i=0 while m_inBuffer entry length > 0"
        ConnectionManager->>NetPacket: reset() then CopyTransportMessage()
        ConnectionManager->>ConnectionManager: processNetCommand or sendRemoteCommand
        ConnectionManager->>Transport: "clear m_inBuffer entry length = 0"
    end
    Note over ConnectionManager: break at first empty slot

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Caller
    participant Connection
    participant Transport
    participant ConnectionManager
    participant NetPacket

    Note over Connection: Static NetPacket allocated once
    Caller->>Connection: sendNetCommandMsg(msg, relay)
    Connection->>NetPacket: reset()
    Connection->>NetPacket: addCommand(tempref)
    alt fits in one packet
        NetPacket-->>Connection: true
        Connection->>Connection: addMessage to m_netCommandList
    else too large to fit
        NetPacket-->>Connection: false
        Connection->>NetPacket: ConstructBigCommandPacketList(tempref)
        Connection->>Transport: queueSend per split packet
    end

    Note over Connection: Static NetPacket reused each loop
    Caller->>Connection: doSend()
    loop while commands remain
        Connection->>NetPacket: reset() then addCommand inner loop
        Connection->>Transport: queueSend(data)
    end

    Transport->>Transport: doRecv - fills m_inBuffer densely from index 0
    Note over Transport: bufferIndex persists within each doRecv call

    Caller->>ConnectionManager: doRelay()
    Note over ConnectionManager: Static NetPacket allocated once
    loop "i=0 while m_inBuffer entry length > 0"
        ConnectionManager->>NetPacket: reset() then CopyTransportMessage()
        ConnectionManager->>ConnectionManager: processNetCommand or sendRemoteCommand
        ConnectionManager->>Transport: "clear m_inBuffer entry length = 0"
    end
    Note over ConnectionManager: break at first empty slot

_{Reviews (2): Last reviewed commit: "Addressed feedback." | Re-trigger Greptile}

xezon

This pull does a number of things.

xezon · 2026-07-11T08:20:06Z

 */
 void ConnectionManager::doRelay() {
+	// this is done so we don't have to allocate and delete a packet every time we relay a message.
+	static NetPacket* packet = newInstance(NetPacket);


It would be better if this was a stack object then

xezon · 2026-07-11T08:23:17Z


 	void init();
 	void reset();
+	void CopyTransportMessage(const TransportMessage& msg);


Or setMessage

xezon · 2026-07-11T08:25:19Z

+	CopyTransportMessage(msg);
+}
+
+void NetPacket::CopyTransportMessage(const TransportMessage& msg)


Better sort it at the right location

xezon · 2026-07-11T08:25:40Z

-	m_addr = msg->addr;
-	m_port = msg->port;
+	m_addr = msg.addr;
+	m_port = msg.port;


This is new. Did not do that before. Is that ok?

xezon · 2026-07-11T08:28:03Z

 				m_transport->m_inBuffer[i].length = 0;
 			}
+		} else {
+			break;


Alternatively could do early breaks in the loops. But its ok either way.

xezon · 2026-07-11T08:33:58Z

 		}
+		else
+		{
+			break;


So there cannot be non-zero buffer after a zero buffer, yes?

xezon · 2026-07-11T08:37:50Z

-	for (Int i = 0; i < MAX_MESSAGES; ++i) {
-		if (m_transport->m_inBuffer[i].length != 0) {
+	for (size_t i = 0; i < ARRAY_SIZE(m_transport->m_inBuffer); ++i) {
+		if (m_transport->m_inBuffer[i].length > 0) {


Status quo: Compares with > 0, != 0

Expectation:

> 0, <= 0

or

== 0, != 0

Caball009 · 2026-07-11T18:17:32Z

This pull does a number of things.

I can put the loop logic in another PR and put #2866 in here to replace all memory pooled uses of NetPacket (either with a single dynamic allocation or stack allocation if possible).

Caball009 added Minor Severity: Minor < Major < Critical < Blocker Performance Is a performance concern Network Anything related to network, servers labels Apr 28, 2026

Caball009 changed the title ~~perf(network): Improve performancy by reducing packet allocations and improving loop logic~~ perf(network): Improve performance by reducing packet allocations and improving loop logic Apr 28, 2026

Caball009 force-pushed the perf_network branch 2 times, most recently from 7ebd8f1 to a2267a1 Compare April 28, 2026 20:41

Caball009 force-pushed the perf_network branch 2 times, most recently from 3fe1916 to d4f75e9 Compare June 2, 2026 20:59

Caball009 added 5 commits July 9, 2026 17:12

Removed unused static (packet and command count) variables.

79aa26c

Reduced the number of packet (re)allocations significantly.

5627337

Refactored while loops to for loops.

922c65b

Improved loop logic performance.

a9382ca

Removed unnecessary (de)allocation.

78a1f7f

Caball009 force-pushed the perf_network branch from d4f75e9 to 78a1f7f Compare July 9, 2026 18:40

Caball009 mentioned this pull request Jul 9, 2026

refactor(network): Remove memory pool for NetPacket #2866

Open

1 task

Caball009 marked this pull request as ready for review July 9, 2026 18:59

greptile-apps Bot reviewed Jul 9, 2026

View reviewed changes

Comment thread Core/GameEngine/Source/GameNetwork/LANAPI.cpp Outdated

Addressed feedback.

35667ec

Caball009 changed the title ~~perf(network): Improve performance by reducing packet allocations and improving loop logic~~ perf(network): Reduce packet allocations and improve loop logic Jul 9, 2026

xezon reviewed Jul 11, 2026

View reviewed changes

Uh oh!

Conversation

Caball009 commented Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO:

Uh oh!

stephanmeesters commented Apr 28, 2026

Uh oh!

Caball009 commented Apr 29, 2026

Uh oh!

Caball009 commented Jun 2, 2026

Uh oh!

greptile-apps Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

Uh oh!

xezon left a comment

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Jul 11, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 commented Jul 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Caball009 commented Apr 28, 2026 •

edited

Loading

greptile-apps Bot commented Jul 9, 2026 •

edited

Loading