Biranavan-Parameswaran commented on PR #2496:
URL: https://github.com/apache/systemds/pull/2496#issuecomment-5055868142
Thanks for the review @ywcb00. All three comments are addressed, and I also
ran experiments to back the design choices. Summary of what changed since the
reviewed commit.
**Comments**
* **Threshold (1):** `INT_MAX-1` can overflow on the wire because routing
uses a size estimate. I measured the overflow cliff at about 1.990 GiB and set
the threshold about 40 MB under it (`2000L<<20`). Full reasoning in the inline
reply.
* **Redundant catch (2):** removed.
* **Rename (3):** `FederatedFormatDetector` is now `FederatedFormatDecoder`.
**Follow up work**
* **Chunk size 4 MB (`1<<22`).** Swept 256K, 1M, 4M and 8M across four
payloads, 48 runs. Median round trip in seconds:
| payload | 256K | 1M | 4M | 8M |
|---|---|---|---|---|
| 0.08 MB | 0.755 | 0.787 | 0.900 | 0.835 |
| 8 MB | 1.208 | 1.165 | 1.099 | 1.066 |
| 512 MB | 10.707 | 10.203 | 10.013 | 10.443 |
| 1074 MB | 17.439 | 16.020 | 15.578 | 15.917 |
4 MB is the robust optimum. It wins on large and xl and stays stable as
the payload doubles. 8M starts to regress on big payloads and 256K pays too
much framing overhead. Small payloads are one frame so they are chunk size
indifferent. It also caps in flight buffering at 16 x 4 MB, so 64 MB.
* **Streaming vs object encoder.** Mean elapsed in seconds, object encoder
vs chunked:
| payload | object encoder | chunked |
|---|---|---|
| tiny | 1.36 | 1.46 |
| mid | 2.16 | 2.21 |
| 512 MB | 74.1 | 18.5 |
| 1 GiB | 305.5 | 29.9 |
Chunked matches the object encoder on small payloads and is 4x to 10x
faster on large ones.
* **Scaling past 2 GB.** Swept 2.2 to 4.0 GB, all correct with 0 exceptions
and 0 OOM:
| payload | round trip s | MB/s | peak worker MB |
|---|---|---|---|
| 2.2 GB | 11.56 | 190 | 5961 |
| 2.6 GB | 14.26 | 182 | 7689 |
| 3.0 GB | 16.14 | 186 | 6744 |
| 4.0 GB | 25.74 | 155 | 9795 |
Throughput stays flat around 155 to 190 MB/s and worker peak memory tracks
payload at about 2.2 to 3.0x with no second full payload buffer. The old object
encoder crashes near 1.99 GiB, so this is the range the codec exists for.
* **Routing** now based on lineage cacheability instead of size. Lineage
cacheable responses use the object encoder, everything else streams. New
`FederatedFormatRoutingTest` covers the four cases.
* **Close race fix.** The worker `CloseListener` treated a client close
during a chunked response drain as a write failure and tried a second write on
the dead channel. It now treats an inactive channel mid stream as a normal end
of stream.
* Slimmed `FederatedChunkEncoder` to a factory and tightened the routing
predicate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
