FWD » Core Development » Runtime Infrastructure

================================================================================================

PROPOSAL - [MINOR] performance - WebClientProtocol.sendBinaryMessageStreamed

The whole payload now accumulates in the push queue, defeating the method's purpose

================================================================================================

REVIEW FINDING (abridged)

   "The transfer's whole payload now accumulates in the push queue, weakening the bounded-footprint

   property the method exists for [...] queueChunk gates only on PushMessagesWorker's 32 MiB *byte*

   high-water mark [...] All ~128 freshly allocated 64 KiB arrays are therefore live in messages at

   once on top of the source pixels, where the old path held one reused

   min(getChunkSizeCached()=1 MiB, bodyLen) buffer."

FILES

   src/com/goldencode/p2j/ui/client/driver/web/WebClientProtocol.java

   src/com/goldencode/p2j/ui/client/driver/web/PushMessagesWorker.java

   src/com/goldencode/p2j/ui/client/gui/driver/web/GuiWebSocket.java

------------------------------------------------------------------------------------------------

1. ROOT CAUSE

------------------------------------------------------------------------------------------------

The pre-rewrite method was strictly bounded: one reused buffer, and a synchronous per-frame

CountDownLatch that stopped the producer until the frame was away. The rewrite had to stop reusing

the buffer - the send is now asynchronous and the queue owns the bytes until the worker writes them,

which the comment at :960-962 states correctly - but it replaced the per-frame stop with the queue's

existing byte budget, and that budget is far larger than one transfer.

queueChunk calls awaitSendCapacity() per piece, which reaches PushMessagesWorker.awaitCapacity():

   if (queuedBytes.get() < highWater())     // DEFAULT_HIGH_WATER = 32 MB

   {

      return;

   }

so the producer does not park until 32 MB is outstanding. The producer is a bulk copy out of

VirtualScreen's int[] running at memory speed; the consumer does one latch-synchronised sendBinary

per message. The producer wins by orders of magnitude, so for any transfer below the high-water mark

the queue accumulates the entire payload and backpressure never engages once.

Peak transient heap for one streamed transfer therefore goes from ~1 MiB to min(bodyLen, ~32 MiB).

Scale, honestly stated: this is bounded, and it is infrequent. defineImageStreamed runs once per

*distinct* image per session behind GuiWebDriver's first-use cache gate, not per frame; createFont

is sub-MB and self-drains because it then blocks in waitForResult. An 8.3 MB full-window background

against a documented ~512 MB client heap is about 1.6%, and 128 short-lived 64 KiB arrays are

cheaper for the collector than the single humongous 8.3 MB array the old non-streaming path used.

So this is a regression against the method's stated design intent rather than a scaling problem -

which is why it is MINOR and why the cheapest correct fix is preferable to a sophisticated one.

------------------------------------------------------------------------------------------------

2. PROPOSED FIX

------------------------------------------------------------------------------------------------

2.1 Gate the streaming producer on in-flight pieces, not on the shared byte budget

------------------------------------------------------------------------------------

A counting semaphore released as each piece is written bounds the transfer to a handful of live

buffers, independently of what the 32 MB budget is doing for ordinary sends. In

PushMessagesWorker, alongside the existing capacity machinery:

   +  /** Maximum streamed pieces outstanding at once; bounds a transfer's live buffers. */

   +  private static final int MAX_INFLIGHT_CHUNKS = 4;

   +

   +  /** Permits for streamed pieces; acquired before queueing one, released once it is written. */

   +  private final Semaphore chunkPermits = new Semaphore(MAX_INFLIGHT_CHUNKS);

   +

   +  /**

   +   * Queue one piece of a streamed transfer, blocking while {@link #MAX_INFLIGHT_CHUNKS} pieces

   +   * are already outstanding.

   +   * <p>

   +   * Unlike {@link #pushMessage} this bounds the transfer by piece count rather than by the shared

   +   * byte budget, so a single large transfer cannot materialize in the queue in its entirety.

   +   * <p>

   +   * IMPORTANT: call this WITHOUT holding {@code WebClientProtocol.lock}.

   +   *

   +   * @param    message

   +   *           The piece to enqueue.

   +   */

   +  public void pushChunk(Object message)

   +  {

   +     try

   +     {

   +        chunkPermits.acquire();

   +     }

   +     catch (InterruptedException e)

   +     {

   +        Thread.currentThread().interrupt();

   +        return;

   +     }

   +

   +     pushMessage(message);

   +  }

The permit must be released wherever a chunk leaves the queue - both after a successful send and on

the failure/teardown paths - or a stalled socket wedges the producer permanently. The cleanest place

is beside the existing releaseCapacity(msgSize) call in the send loop, conditional on the message

being a chunk. That needs the worker to recognise a chunk; the least invasive way is to wrap the

piece in a tiny marker type rather than inspecting msg[0]:

   +  /** A streamed transfer piece; carries a permit that is released once the piece is written. */

   +  static final class Chunk

   +  {

   +     final ByteBuffer buffer;

   +

   +     Chunk(ByteBuffer buffer) { this.buffer = buffer; }

   +  }

with sizeOf() and the send path unwrapping it, and releaseCapacity's neighbourhood doing

chunkPermits.release() for a Chunk. stopPushing() must drain and release outstanding permits too.

Then in WebClientProtocol.queueChunk, swap the call:

   -        pushWorker.pushMessage(ByteBuffer.wrap(msg, 0, CHUNK_HEADER_SIZE + payloadLen));

   +        pushWorker.pushChunk(ByteBuffer.wrap(msg, 0, CHUNK_HEADER_SIZE + payloadLen));

and note that the acquire must happen outside synchronized (lock) - move it to sit with the existing

awaitSendCapacity() call at the top of queueChunk rather than inside the block, for the same

deadlock reason that comment already gives.

At MAX_INFLIGHT_CHUNKS = 4 the peak is ~256 KiB of live buffers instead of the whole payload, and

the heartbeat still interleaves freely because pushMessageFirst neither acquires a permit nor waits

behind one.

2.2 Cheaper alternative if 2.1 is judged too much machinery

-------------------------------------------------------------

Give the streamed path its own, much lower byte threshold rather than a permit count:

   in queueChunk, replace awaitSendCapacity() with a variant that parks while queuedBytes exceeds,

   say, 1 MB rather than the 32 MB high-water mark.

This needs no marker type and no permit accounting - just a second threshold parameter on

awaitCapacity - but it is approximate, because queuedBytes is shared with ordinary sends, so a busy

draw batch can make the streamed producer park early. That is harmless (it only throttles a

background transfer) and this option is a legitimate 80% fix at 20% of the complexity.

Recommendation: take 2.2 unless a measurement shows the transfer footprint actually matters, and

record 2.1 as the design if it ever does. The finding is a design-intent regression, not a

production symptom, and 2.1 adds a marker type and permit-lifecycle obligations to a class that is

already the most concurrency-sensitive in this area.

2.3 Correct the stale javadoc uncovered alongside this

--------------------------------------------------------

GuiWebSocket.defineImageStreamed's javadoc parenthetical still says "the send blocks until the whole

message is on the wire". The *requirement* it justifies still holds - sendBinaryMessageStreamed

drains the InputStream on the calling thread, which is what keeps the FWD context available for the

underlying chunk pulls - but the stated reason has not been true since the rewrite. Reword to say

the body is consumed on the calling thread, without claiming the send is synchronous.

------------------------------------------------------------------------------------------------

3. RELATIONSHIP TO OTHER PROPOSALS

------------------------------------------------------------------------------------------------

   proposal-issue-critical-functional-1   changes queueChunk's signature and adds a generation

                                          check.  If both are taken, apply that one first; the

                                          permit acquire belongs next to awaitSendCapacity(), before

                                          the generation check, and an abandoned transfer must

                                          release any permit it holds.

Note the interaction explicitly: with 2.1, a transfer abandoned by the generation check returns from

queueChunk having acquired a permit it never queues. That permit must be released on that path or

repeated reconnects will exhaust the semaphore and hang the next transfer.

------------------------------------------------------------------------------------------------

4. RISK AND VERIFICATION

------------------------------------------------------------------------------------------------

Risk: moderate for 2.1 - a permit leak on any path that removes a chunk from the queue without

releasing (teardown, discardQueued from the critical proposal, an exception in the send loop) turns

into a hung producer, which is worse than the footprint it fixes. Low for 2.2, which cannot deadlock

because it reuses the existing timed-wait capacity mechanism.

Verification

   1. Baseline the peak: attach a heap sampler (or log queuedBytes at each queueChunk) while pushing

      a maximized-window background image. Expected pre-fix: queuedBytes tracks the full payload.

      Post-fix (2.1): it plateaus at ~4 pieces; (2.2): at ~1 MB.

   2. Confirm the transfer still completes and renders correctly, and time it - the fix must not

      measurably slow the transfer, since the consumer was never the bottleneck's cause.

   3. Confirm the heartbeat still goes out during a large transfer (server log shows pings at the

      configured cadence throughout), i.e. the permit gate did not starve pushMessageFirst.

   4. For 2.1 specifically: kill the link mid-transfer, let the session tear down and reconnect, and

      repeat five times. Confirm the next transfer still starts - this is the permit-leak test and

      is the one that matters.

   5. Confirm custom font loading (the other sendBinaryMessageStreamed caller) is unaffected.