perf(network): Improve performance by refactoring loop logic and removing unused static variables

[Caball009]

This PR makes modest performance improvements:

1. Improves loop logic in a couple of places (early breaks and keep track of the current index to avoid restarting inner loops).
2. Removes unused static variables.

See commits for cleaner diffs.

TODO:

• Verify everything works ok.

[stephanmeesters]

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]

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]

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]

Greptile Summary

This PR delivers modest network-layer performance improvements by refactoring loop logic across four files and removing two unused debug counters (numPackets, numCommands) in ConnectionManager::doRelay().

• Early break on empty slot: Consumer loops in ConnectionManager::doRelay(), NAT::connectionUpdate(), and LANAPI::update() now break as soon as they hit a zero-length buffer entry, relying on the invariant that m_inBuffer is always a contiguous prefix of filled slots.
• Persistent bufferIndex: Transport::doRecv() and Transport::doSend() (latency path) now carry bufferIndex across inner-loop iterations so each successive message doesn't rescan already-occupied slots from index 0; the assert previously commented-out is re-enabled as DEBUG_ASSERTCRASH(bufferIndex < MAX_MESSAGES, ...).
• ARRAY_SIZE() over MAX_MESSAGES: All array-bound literals are replaced with the safer compile-time macro; all three arrays (m_inBuffer, m_outBuffer, m_delayedInBuffer) are declared as [MAX_MESSAGES], so the substitution is numerically equivalent.

Confidence Score: 5/5

Changes are safe to merge. The early-break optimization is correct because each consumer calls m_transport->update() (which runs doRecv()) immediately before its own loop, guaranteeing a freshly-filled contiguous buffer by the time the break logic runs.

The three core invariants the PR relies on all hold: doRecv() fills m_inBuffer from index 0 each call, consumers iterate and clear sequentially from index 0, and all three arrays share the same MAX_MESSAGES bound so ARRAY_SIZE() substitution is numerically identical. Removing the dead static counters is a clean, risk-free change. The re-enabled DEBUG_ASSERTCRASH fires correctly for the first packet that will actually be dropped.

Transport.cpp deserves the closest review — the bufferIndex carrying state across the outer while loop is a new and non-obvious pattern; a comment explaining the invariant it relies on would help future readers.

Important Files Changed

Filename	Overview
Core/GameEngine/Source/GameNetwork/Transport.cpp	Most complex change: introduces shared bufferIndex across while-loop iterations in doRecv() and inner loop in doSend() latency path. Logic is correct given all three arrays are MAX_MESSAGES in size, and continue after the latency block gives proper mutual exclusion. The re-enabled DEBUG_ASSERTCRASH fires correctly for the actual first lost packet.
Core/GameEngine/Source/GameNetwork/ConnectionManager.cpp	Removes dead numPackets/numCommands static counters; converts while-loops to for-loops; adds early break. packet is set to nullptr immediately after deleteInstance inside the first loop, so the deleteInstance(packet) after the second loop is a safe null call.
Core/GameEngine/Source/GameNetwork/NAT.cpp	Adds early break on empty slot. Safe because NAT calls m_transport->update() immediately before the loop, guaranteeing a freshly-filled contiguous buffer. Unrecognized packets (neither PROBE nor KEEPALIVE) are never cleared — a pre-existing issue that does not interact badly with the break since uncleared slots keep length > 0.
Core/GameEngine/Source/GameNetwork/LANAPI.cpp	Minor change: variable declaration moved into for-init, ARRAY_SIZE() substituted, else { break; } added on empty slot. The !LANbuttonPushed guard in the loop condition is correctly retained. Every branch (including default) ends by setting m_inBuffer[i].length = 0, so LANAPI always leaves a clean buffer for the next frame's doRecv().

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant GameLoop
    participant Transport
    participant Consumer

    GameLoop->>Transport: update()
    activate Transport
    Transport->>Transport: doRecv - fills m_inBuffer[0..k] contiguously
    note right of Transport: bufferIndex resets to 0 each call,<br/>skips full slots, fills only empty ones
    Transport->>Transport: doSend - delivers delayed msgs to m_inBuffer[k+1..m]
    deactivate Transport

    GameLoop->>Consumer: doRelay / connectionUpdate / LANAPI::update
    activate Consumer
    Consumer->>Consumer: "iterate i=0 while length > 0"
    Consumer->>Consumer: "clear m_inBuffer[i].length = 0"
    Consumer->>Consumer: else break on first empty slot (NEW)
    deactivate Consumer

    note over Transport,Consumer: Invariant: m_inBuffer is always a contiguous filled prefix.<br/>Break is safe only because doRecv fills from index 0<br/>and consumers clear sequentially.

Loading

%%{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 GameLoop
    participant Transport
    participant Consumer

    GameLoop->>Transport: update()
    activate Transport
    Transport->>Transport: doRecv - fills m_inBuffer[0..k] contiguously
    note right of Transport: bufferIndex resets to 0 each call,<br/>skips full slots, fills only empty ones
    Transport->>Transport: doSend - delivers delayed msgs to m_inBuffer[k+1..m]
    deactivate Transport

    GameLoop->>Consumer: doRelay / connectionUpdate / LANAPI::update
    activate Consumer
    Consumer->>Consumer: "iterate i=0 while length > 0"
    Consumer->>Consumer: "clear m_inBuffer[i].length = 0"
    Consumer->>Consumer: else break on first empty slot (NEW)
    deactivate Consumer

    note over Transport,Consumer: Invariant: m_inBuffer is always a contiguous filled prefix.<br/>Break is safe only because doRecv fills from index 0<br/>and consumers clear sequentially.

Loading

_{Reviews (3): Last reviewed commit: "Used explicit 'length > 0' check instead..." | Re-trigger Greptile}

[xezon]

This pull does a number of things.

[xezon]

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

[xezon]

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

[Caball009]

That's right. Transport::doRecv should handle that correctly.

[xezon]

Status quo: Compares with > 0, != 0

Expectation:

> 0, <= 0

or

== 0, != 0

[Caball009]

I've split this off to a separate commit. I do think this is the right comparator, though, as long as length is still signed.

The original code already does it this way here:

GeneralsGameCode/Core/GameEngine/Source/GameNetwork/LANAPI.cpp

Line 347 in ca9358f

if (m_transport->m_inBuffer[i].length > 0)

GeneralsGameCode/Core/GameEngine/Source/GameNetwork/NAT.cpp

Line 329 in ca9358f

if (m_transport->m_inBuffer[i].length > 0) {

[Caball009]

I can change the == 0 cases to <= 0 if you like.

[Caball009]

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).

[xezon]

Whatever makes most sense.