[perf] Pack serially in YuanrongStorageClient (DS_MAX_WORKERS 16 -> 1) - #153
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c6285c089
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| PUT_KEYS_LIMIT: int = 10_000 | ||
| GET_CLEAR_KEYS_LIMIT: int = 10_000 | ||
| DS_MAX_WORKERS: int = 16 | ||
| DS_MAX_WORKERS: int = 1 |
There was a problem hiding this comment.
When Yuanrong stores general CPU-side values whose packed buffers are large, such as image-sized CPU tensors/ndarrays sent through kv_batch_put, forcing the default to one worker disables the existing parallel pack_into path for every batch. These values are accepted by the public KV/general-object path, and the commit's own size sweep shows serial packing becomes slower past roughly 256 KiB per object (about 1.5x slower at 512 KiB), so an adaptive threshold or configurable default would keep the small-object win without regressing supported large-value workloads.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
|
CC @dpj135 |
CLA Signature Guide@Chase-Rong , thanks for your pull request. The following commit(s) are not associated with a signed Contributor License Agreement (CLA).
To sign CLA, click here. To check if your email is configured correctly, refer to the FAQs. Once you've signed the CLA or updating your email, please comment |
3 participants
Problem
batch_encode_intoonly parallelisespack_into; the msgpack encoding itself isalways serial.
pack_intois a memcpy of a few hundred bytes per object — at TQ'sobject sizes it is dominated by Python/ATen dispatch, which holds the GIL. Handing
that to a
ThreadPoolExecutordoes not overlap anything; the threads serialise on theGIL and the pool's per-task cost is added on top.
Measured on a 512-sample put (1024 objects), Ascend NPU container:
The per-call inflation is the tell: the same function on the same input takes 38.5x
longer per invocation once 16 threads contend for the GIL. Effective parallelism is
~10.9x, but each call is 38.5x slower, so the phase ends up 3.5x slower than just
doing it in order. At 1024 samples the picture is identical: 104.7ms of work takes
360.0ms across 16 threads (3.44x slower).
Change
DS_MAX_WORKERS: int = 16->1, which takesbatch_encode_intodown its existingserial path. It stays a class attribute, so a deployment that measures a different
trade-off can override it.
Measurements
verl GRPO on
mainat 750719e, this PR applied alone (the other two perf branchesreverted to baseline). Medians over 4-7 calls; two independent baseline runs per
configuration:
put_datayuanrong RPC time is unchanged (32.1 -> 31.4ms single-node, 55.2 -> 61.3ms dual-node),
so the saving is entirely client-side CPU.
get_datais unaffected as expected(medians within 3% across variants).
Two notes on reading these numbers:
tq_mscomponent rises (269.5 -> 133.2 is the TQ-side total, but thepack_intolabel goes from 12.0 to 51.4ms) because packing stops being spreadacross threads and becomes real serial time. The phase wall clock is what drops.
put_datawall clock is not a reliable metric in this environment: anintermittent ~480ms
run_in_executorhandoff delay appears in some runs(two baseline runs measured 606ms and 1083ms with every instrumented step within
1%, the difference landing entirely in un-instrumented time). Judge the dual-node
effect by TQ-side total or pack phase, which agree to 0.2%/0.9% between those two
runs.
How the ratio scales
The ratio is flat in object count and decays in object size — it does not grow in
either direction:
Both paths scale linearly with count (8x the objects gives 7.6x / 8.05x the time), so
growing the cluster or the global batch increases the absolute saving
proportionally — 137ms at 1024 objects, 260ms at 2048 — while leaving the ratio
unchanged. Object size is the only axis that erodes the ratio, per the table above:
3.6x at 512B, 2.6x at 32KB, break-even at 256KB, and a net loss beyond that.
In short: this optimisation is sized by how big each object is, not by how many.
TQ's one-key-per-(sample, field) layout keeps objects in the 512B-8KB range, which is
where the serial path is furthest ahead.
Applicability: this wins because TQ's objects are small
The saving comes from removing GIL contention, whose cost is roughly fixed per
map()call, while the work being parallelised grows with object size. So the winshrinks as objects get bigger, and eventually reverses. Swept on the same machine,
object count fixed at 1024:
The mechanism is visible in the two middle columns: across a 1024x range of object
sizes the 16-worker wall clock moves only 66% (147 -> 245ms), because it is dominated
by a fixed contention cost, while the serial wall clock tracks real work and grows
8.9x. The ratio narrows because serial catches up, not because the threads get better.
Past ~256KB the memcpy is long enough to release the GIL productively and the pool
starts paying off.
Where TQ sits today: keys are
global_index@field_name, one key per (sample,field), so objects are small. The production put measured in this run costs 50.4us per
object serially, which lands between the 512B and 8KB rows above — i.e. in the flat
region where serial is 3.3-3.7x ahead, far from break-even.
By object count the ratio is stable, so this does not decay as the cluster or the
global batch grows (512B objects, 1 worker as reference):
Both paths scale linearly (8x the objects gives 7.6x / 8.05x the time), so a larger
batch increases the absolute saving proportionally and leaves the ratio intact.