-
Notifications
You must be signed in to change notification settings - Fork 45
[perf] Reclaim cleared keys by expiring them instead of deleting #156
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -113,6 +113,14 @@ backend: | |
| metastore_port: 2379 | ||
| # Whether to enable npu transport | ||
| enable_yr_npu_transport: false | ||
| # 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. | ||
| # 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| # Whether to enable host RDMA (H2H) transport via UCX. Requires RDMA NIC hardware and rdma-core driver. | ||
| # See https://pages.openeuler.openatom.cn/openyuanrong-datasystem/docs/zh-cn/latest/best_practices/best_practices_for_rdma.html | ||
| enable_rdma: false | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -198,6 +198,7 @@ class GeneralKVClientAdapter(StorageStrategy): | |
| PUT_KEYS_LIMIT: int = 10_000 | ||
| GET_CLEAR_KEYS_LIMIT: int = 10_000 | ||
| DS_MAX_WORKERS: int = 16 | ||
| CLEAR_EXPIRE_SECOND: int = 1 | ||
|
|
||
| def __init__(self, config: dict): | ||
| port = config.get("worker_port") | ||
|
|
@@ -214,6 +215,10 @@ def __init__(self, config: dict): | |
| ) | ||
| logger.info(f"Using auto-detected host: {host}") | ||
|
|
||
| # 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. self._ttl_second = int(config.get("data_ttl_second", 0)) |
||
|
|
||
| self._ds_client = datasystem.KVClient(host, port) | ||
| self._ds_client.init() | ||
| logger.info("YuanrongStorageClient: Create KVClient to connect with yuanrong-datasystem backend!") | ||
|
|
@@ -256,10 +261,13 @@ def supports_clear(self, strategy_tag: str) -> bool: | |
| return isinstance(strategy_tag, str) and strategy_tag == self.strategy_tag() | ||
|
|
||
| def clear(self, keys: list[str]) -> None: | ||
| """Delete keys in batches.""" | ||
| """Release keys in batches, expiring them if a TTL is configured.""" | ||
| for i in range(0, len(keys), self.GET_CLEAR_KEYS_LIMIT): | ||
| batch_keys = keys[i : i + self.GET_CLEAR_KEYS_LIMIT] | ||
| self._ds_client.delete(batch_keys) | ||
| if self._ttl_second: | ||
| self._ds_client.expire(batch_keys, self.CLEAR_EXPIRE_SECOND) | ||
| else: | ||
| self._ds_client.delete(batch_keys) | ||
|
|
||
| def mset_zero_copy(self, keys: list[str], objs: list[Any]): | ||
| """Store multiple objects in zero-copy mode using parallel serialization and buffer packing. | ||
|
|
@@ -273,7 +281,11 @@ def mset_zero_copy(self, keys: list[str], objs: list[Any]): | |
| def alloc(sizes): | ||
| # DataSystem buffers must be converted via MutableData() to obtain | ||
| # a memoryview-compatible data structure for zero-copy packing. | ||
| mcreate_bufs = self._ds_client.mcreate(keys, sizes) | ||
| # Passing the TTL here is what makes clear's expire() safe: a cleared | ||
| # 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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if |
||
| buffers.extend(mcreate_bufs) | ||
| return [buf.MutableData() for buf in mcreate_bufs] | ||
|
|
||
|
|
@@ -485,6 +497,20 @@ def _route_to_strategies( | |
| A dictionary mapping each active strategy to a list of indexes in `items` | ||
| that it should handle. Every index appears exactly once. | ||
| """ | ||
| # Backend-meta tags are hashable and, for a batch written by one backend, | ||
| # all identical - so one selector call can decide the whole batch, for example | ||
| # Skipping the per-item loop takes a 1024-sample x 20-field clear from ~20k | ||
| # selector calls down to one. Mixed or unmatched tags fall through to the loop below. | ||
| if item_label == self.ROUTE_ITEM_AS_BACKEND_META: | ||
| distinct = set(items) | ||
| if len(distinct) == 1: | ||
| tag = distinct.pop() | ||
| owner = next((s for s in self._strategies if selector(s, tag)), None) | ||
| if owner is not None: | ||
| routed = {s: [] for s in self._strategies} | ||
| routed[owner] = list(range(len(items))) | ||
| return routed | ||
|
|
||
| unmatched_count = 0 | ||
| warning_count = 0 | ||
| routed_indexes: dict[StorageStrategy, list[int]] = {s: [] for s in self._strategies} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -809,5 +809,12 @@ async def clear_data(self, metadata: BatchMeta) -> None: | |
| ) | ||
|
|
||
| keys = self._generate_keys(metadata.field_names, metadata.global_indexes) | ||
| _, _, custom_meta = self._get_shape_type_custom_backend_meta_list(metadata) | ||
| # 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. | ||
|
Comment on lines
+812
to
+814
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Annotations should be based on the current code rather than the code changes. |
||
| custom_meta = [ | ||
| per_sample.get(field_name) | ||
| for field_name in sorted(metadata.field_names) | ||
| for per_sample in metadata._custom_backend_meta | ||
| ] | ||
| self.storage_client.clear(keys=keys, custom_backend_meta=custom_meta) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This new seven-line comment includes the workload-specific
~3x fasterbenchmark, 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 👍 / 👎.