Reduce memory use of the waiting_tasks metric - #8009
Conversation
aKlimau
changed the title
aKlimau
changed the title
a68db73 to
21abd31
Compare
|
After discussing all the suggested solutions in #7986 with claude, it looks like solutions 2 and 3 would have a positive impact on memory usage with no behavioral change. METRIC_HEARTBEAT_INTERVAL is a separate change that would make metrics not as fresh, so we'd need to decide weather it's worth. And using Redis cache doesn't save us much, since the computation is always done by a single worker and runs once per cycle. So introducing a round trip and another potential point of failure doesn't seem worth it to me. |
|
@decko by any chance do we have a way to measure the performance impact of such changes? (Memory allocation during count_waiting_tasks_for_metric() call) |
| """ | ||
| Split a ``reserved_resources_record`` array into (exclusive, shared) resource names. | ||
|
|
||
| Shared reservations are stored with a ``"shared:"`` prefix; exclusive ones are bare. |
There was a problem hiding this comment.
Get rid of the double backticks please
The metric scans every waiting/running task but only needs each task's reserved resources. Fetch just those instead of building a model object per row. ref pulp#7986 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
21abd31 to
72bfa27
Compare
| ) | ||
| .order_by("pulp_created") | ||
| .only("reserved_resources_record") | ||
| .values_list("reserved_resources_record", flat=True) # Avoids Task object allocation |
There was a problem hiding this comment.
The comment is not needed. Everyone working on the codebase is expected to know how django and values_list works.
|
|
||
| Shared reservations are stored with a `"shared:"` prefix; exclusive ones are bare. | ||
| The prefix is stripped from the returned shared names. | ||
|
|
There was a problem hiding this comment.
| Shared reservations are stored with a `"shared:"` prefix; exclusive ones are bare. | |
| The prefix is stripped from the returned shared names. |
The code is pretty straightforward, not sure we need a comment describing it.
| @@ -0,0 +1 @@ | |||
| Reduced the memory overhead of the Redis worker `waiting_tasks` metric by streaming reserved-resource records instead of loading full `Task` objects. | |||
There was a problem hiding this comment.
I would mark this as a misc. It's not really a feature and we can backport if need be.
We do have an opentelemetry profile on oci-env. It starts the opentelemetry-collector, Prometheus and Grafana. You could try to use it and check the worker memory consumption. |
|
Benchmarked this before/after to check it's actually worth the change. Ran both implementations back to back in one process against the same seeded tasks in a dev env. Best of 3 for time, separate pass for peak memory. Both implementations return identical counts.
6-14x faster, and peak memory goes from linear in queue depth to flat. Distinct resource count doesn't matter - 10k tasks over 50 resources and over 10k resources come out about the same. |
4 participants
count_waiting_tasks_for_metric() now streams reserved_resources_record via values_list().iterator() instead of materializing deferred Task instances.
The "shared:" split is moved into redis_locks._split_reserved_resources().
ref #7986
📜 Checklist
See: Pull Request Walkthrough