[perf] Reclaim cleared keys by expiring them instead of deleting - #156
[perf] Reclaim cleared keys by expiring them instead of deleting#156Chase-Rong wants to merge 1 commit into
Conversation
A kv_clear costs more than a put: measured on a verl PPO run, 493 ms for 512
samples and 1260 ms for 1024, of which yuanrong's delete is 83%. delete does not
parallelise (16 threads and 8 separate connections both come out slower than
serial, so the serialisation is server-side), and it gets more expensive the
further the data is: 28.7 us/key when the key was written locally, 38.0 us/key
when another node wrote it. That is what made clear superlinear across nodes -
2.55x for twice the batch.
datasystem's expire is ~3x cheaper than delete and, being a metadata mark, does
not pay the remote hop: 9.2 us/key whether one node or two. So clear now expires
keys when a TTL is configured, and scaling returns to 2.01x.
Doing this safely needs both halves of the change. clear_meta returns cleared
global_indexes to the reusable pool, so a later put rebuilds the very same
'{index}@{field}' keys - and a TTL survives a rewrite unless the rewrite states
one explicitly (ttl_second=0 means "delete manually", which the backend reads as
"leave the existing deadline alone", not "clear it"). mcreate therefore passes
the configured TTL on every write, re-arming the clock. Expiring in clear without
that would silently delete freshly written training data.
Visibility is unchanged. For the KV interface, reachability is decided by the
controller's keys_mapping, not by production_status - kv_retrieve_meta and
get_data never consult it - so once clear_meta has run the keys cannot be named,
whether or not the bytes are still resident. The TTL only bounds when the space
comes back, and it also reclaims keys orphaned by a crash. In the pre-existing
window between the storage call and clear_meta, a concurrent read now sees intact
old data rather than the None it would get from a completed delete.
data_ttl_second: 0 keeps the synchronous delete. DsTensorClient offers neither
expire nor a TTL argument, so the NPU tensor path keeps deleting; clear already
routes per strategy.
Two smaller wins on the same path, both scaling with batch size:
- route once per distinct backend tag instead of once per item, since a batch
written by one backend carries one tag (12.8 -> 1.6 ms at 28672 keys)
- stop building shapes/dtypes in clear_data, which only needs routing tags
(8.4 -> 0 ms)
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 |
![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: 6f021b4c1f
ℹ️ 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".
| # orphaned by a crash. Set to 0 to delete synchronously instead. | ||
| # Must exceed the lifetime of any single batch. Ignored by the NPU tensor path, | ||
| # which datasystem does not offer a TTL for. | ||
| data_ttl_second: 120 |
There was a problem hiding this comment.
Setting this default to 120 makes every general Yuanrong value expire two minutes after its write even when it remains live in controller metadata; reads do not refresh the deadline. Any supported workload that queues or retains a batch for longer than two minutes will therefore receive missing values without ever clearing them, whereas the previous default retained data until an explicit clear. Keep the default at 0 unless the lifecycle can enforce a safe upper bound.
AGENTS.md reference: AGENTS.md:L17-L18
Useful? React with 👍 / 👎.
| # Lifetime in seconds written onto every stored key, refreshed on each write. | ||
| # When > 0, clear marks keys to expire instead of deleting them, which datasystem | ||
| # does ~3x faster; keys become unreachable as soon as clear releases the metadata, | ||
| # and the TTL only bounds when the space is reclaimed. It also cleans up keys | ||
| # orphaned by a crash. Set to 0 to delete synchronously instead. |
There was a problem hiding this comment.
This new seven-line comment includes the workload-specific ~3x faster benchmark, while the repository requires comments to be concise 2–4-line explanations of why and explicitly forbids single-run metrics. Reduce it to the stable TTL/reclamation semantics and safety constraint so the shipped configuration does not retain stale benchmark evidence.
AGENTS.md reference: AGENTS.md:L24-L26
Useful? React with 👍 / 👎.
|
I think it is better for |
| # orphaned by a crash. Set to 0 to delete synchronously instead. | ||
| # Must exceed the lifetime of any single batch. Ignored by the NPU tensor path, | ||
| # which datasystem does not offer a TTL for. | ||
| data_ttl_second: 120 |
There was a problem hiding this comment.
I think it is better for data_ttl_second to default 0. Users should configure data_ttl_second based on their usage scenarios. 120 is too magic.
| # Clear only routes by backend tag; asking for shapes/dtypes here would | ||
| # build two more B*F lists just to discard them. Field-major order to | ||
| # match _generate_keys. |
There was a problem hiding this comment.
Annotations should be based on the current code rather than the code changes.
|
|
||
| # Written onto every key and refreshed by each write, so a reused key never | ||
| # inherits an older deadline. 0 keeps the synchronous-delete behaviour. | ||
| self._ttl_second = int(config.get("data_ttl_second", 0) or 0) |
There was a problem hiding this comment.
self._ttl_second = int(config.get("data_ttl_second", 0))
| # global_index goes back into the reusable pool, so a later put rebuilds | ||
| # the same key, and only an explicit TTL resets the deadline (0 leaves an | ||
| # earlier one in place, which would delete the data we just wrote). | ||
| mcreate_bufs = self._ds_client.mcreate(keys, sizes, ttl_second=self._ttl_second) |
There was a problem hiding this comment.
if self._ttl_second=0 ,
will mcreate_bufs be deleted immediately?
3 participants
Background
Measured on a verl PPO run (
main_ppo_sync, Yuanrong backend, 28 fields per sample), with a probe splitting the call into its four legs —KV_RETRIEVE_META→MARK_CLEARING→ storage release →CLEAR_META:deleteA clear touches
B × Fkeys — 28672 of them at 1024 samples — and datasystem'sdeleteis priced per key.Why clear was superlinear across nodes
deletegets more expensive the further the data is:deleteon a key written by…In a 2-node run roughly half the keys live on the remote node, and the driver deletes them all from one process, so every remote key pays a forwarding hop. The batch doubles and the unit price rises — hence 2.55x wall clock for 2x the work.
This is the distinction that matters: the extra time is not "more work", it is "each unit of work got more expensive". Per-key cost separates the two; a wall-clock ratio on its own cannot.
expireis a metadata mark. It never touches the node holding the data, so it costs 9.2 us/key on one node and 9.2 us/key on two — identical. Switchingcleartoexpirebrings scaling back to 2.01x.What changed
A new
backend.Yuanrong.data_ttl_second(default 3600, 0 keeps the currentsynchronous delete):
GeneralKVClientAdapter.clearcallsexpire(keys, CLEAR_EXPIRE_SECOND=1)when a TTL is configured, anddeletewhen it is not.mset_zero_copypasses that TTL tomcreateon every write.Both halves are required, and this is the subtle part of the change.
clear_metareturns clearedglobal_indexes to the reusable pool, so a laterputrebuilds the exact same{index}@{field}keys — and a TTL survives a rewrite unless the rewritestates one explicitly.
ttl_second=0means "delete manually", which the backend reads as "leave the existing deadline alone", not "clear it". Verified directly:ttl_second=600ttl_second=0(a plain put)So expiring in
clearwithout passing a TTL on writes would silently delete freshly written training data — and because a clear is ~60 s away from the nextputin practice while the deadline is 1 s, it would almost never reproduce.The TTL argument itself is free:
mcreatewith a TTL is 66.0 ms vs 78.5 ms without,i.e. within noise.
Two smaller wins on the same path, both scaling with batch size:
_route_to_strategiesresolved the owning strategy once per item; a batch written by one backend carries one distinct tag, so it now decides once per distinct tag (12.8 → 1.6 ms at 28672 keys). Mixed tags, unmatched tags and thegetpath'signore_unmatched=Falseraise all fall through to the original loop.clear_dataasked_get_shape_type_custom_backend_meta_listfor a triple but only needs the routing tags; shapes/dtypes were built and discarded (8.4 → 0 ms).Results
Full verl PPO runs, 6 steps per configuration, medians of 6
kv_clearsamples. The cluster topology is matched to the node count (p2 detached for the 1-node rounds), and the two variants of a given node count run back to back.retrieve_meta(rpc)route_to_strategiesget_shape_typedeleteexpiredeleteexpireScaling 1 node → 2 nodes (2.00x is perfectly linear):
Run-to-run spread also tightens, because the remaining cost no longer depends on where
the data sits:
The first clear of every base round is a cold-start outlier (748 ms / 2099 ms), which
is why medians are used throughout.
Correctness
Visibility is unchanged. For the KV interface, reachability is decided by the controller's
keys_mapping, not byproduction_status:async_kv_batch_getiskv_retrieve_meta(create=False)+get_data, and neither consults it (the symbol does not appear instorage/managers/base.pyat all).mark_clearingtherefore guards the sampler path only — the boundary for KV callers isclear_meta. Once that has run the keys cannot be named, whether or not the bytes are still resident. The TTL only bounds when the space comes back.No stale reads. Between the storage call and
clear_metathere is a pre-existing non-atomic window. Withdeletea concurrentkv_batch_getin that window readsNone; withexpireit reads intact old data. Consistency is slightly better, and the window shrinks from 389 ms to 134 ms.Partial rewrites are safe. If a cleared index is reused and the new
putonly writes some fields, the old5@Bis still resident until its deadline. It is unreachable:_generate_keysonly builds keys for the new metadata'sfield_names, andclear_metaalready dropped the old fields viafield_meta.remove_samples. Metadata is the sole source of key reachability, so the chain is closed.Crash cleanup comes for free: keys orphaned by a killed process now expire instead
of leaking.
NPU tensor path is untouched.
DsTensorClientoffers neitherexpirenor a TTL argument, so it keeps deleting;clearalready routes per strategy, so the two paths separate on their own.The TTL must exceed the lifetime of any single batch — noted in the config comment.
Testing
tests/test_yuanrong_client_zero_copy.py: 3 new cases —cleardispatches toexpire/deleteperdata_ttl_secondand the TTL reachesmcreate(parametrised over 0 and 3600), plus batching pastGET_CLEAR_KEYS_LIMIT.test_mset_mget_p2p'smcreatemock signature updated for the new keyword. 4 passed in-container.data_ttl_second=0deletes at 24.7 us/key and keys vanish immediately (byte-for-byte the old behaviour);data_ttl_second=3600expires at 9.9 us/key, rewritten keys survive 1000/1000 and cleared keys are reclaimed._route_to_strategiesfast path against the loop across 12 cases: single tag, mixed tags, unsupported tag, empty tags, one-tag-plus-empty, empty input, single strategy, thegetpath'signore_unmatched=False(ValueErrorpreserved), and theputpath's value routing.round: the 1-node logs contain zero references to the second node.
python -m compileall -q transfer_queue tutorial testsclean.