Skip to content

refactor(memory-pool): drain the table in cleanup_all instead of cloning keys - #2840

Merged
trunk-io[bot] merged 1 commit into
mainfrom
claude/dreamy-bardeen-0efkxi-memory-pool-cleanup
Jul 28, 2026
Merged

refactor(memory-pool): drain the table in cleanup_all instead of cloning keys#2840
trunk-io[bot] merged 1 commit into
mainfrom
claude/dreamy-bardeen-0efkxi-memory-pool-cleanup

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Issue

MemoryPoolManager::cleanup_all collected every MemoryPoolId key into a throwaway Vec (cloning two Strings per entry) only to look each id back up and remove it one at a time:

let ids: Vec<MemoryPoolId> = table.keys().cloned().collect();
...
for id in &ids {
    if let Some(entry) = table.remove(id) ...
}

Fix

Replace that with a single std::mem::take(&mut *table) move and iterate the drained map directly, dropping the key clones and the redundant per-id re-lookup.

Behavior is unchanged:

  • the guard is still held for the whole function (free_shared_memory never re-enters the table, so this can't deadlock — the original relied on the same fact);
  • the table still ends empty;
  • the unreleased_count / released_count accounting is identical.

Validation

  • cargo clippy -p dora-memory-pool --all-targets -- -D warnings clean.
  • cargo test -p dora-memory-pool --lib → 9 passed, including cleanup_all_tracks_counts and cleanup_all_reports_partial_release_on_failure.

⚠️ This PR is machine-generated by Claude (an AI assistant) as part of an automated code-review run, and was verified against current origin/main. Please review carefully before merging.

🤖 Generated with Claude Code

https://claude.ai/code/session_01TMv79fayzGF9mDV7vPMQN7


Generated by Claude Code

…ing keys

`cleanup_all` collected every `MemoryPoolId` key into a throwaway `Vec`
(cloning two `String`s per entry) only to look each id back up and
`remove` it one at a time. Replace that with a single
`std::mem::take(&mut *table)` move and iterate the drained map directly.

Behavior is unchanged: the guard is still held for the whole function
(`free_shared_memory` never re-enters the table, so this can't
deadlock), the table still ends empty, and the `unreleased_count` /
`released_count` accounting is identical.

This change is machine-generated by Claude (an AI assistant) as part of an
automated code-review run, and reviewed against current origin/main.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TMv79fayzGF9mDV7vPMQN7
@trunk-io

trunk-io Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

😎 Merged successfully - details.

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — fully automated, no human in the loop.

The std::mem::take(&mut *table) drain in cleanup_all is behavior-equivalent to the previous clone-keys-then-remove loop: the lock guard is held for the same window, unreleased_count is unchanged (drained.len() == prior ids.len()), the table still ends empty, and free_shared_memory does not re-enter the table (it was already called under the same guard in the old code, so a re-entrant access would have deadlocked before). Avoids the per-entry key clones. No issues found.


Generated by Claude Code

@phil-opp
phil-opp marked this pull request as ready for review July 28, 2026 10:42
@trunk-io
trunk-io Bot merged commit 59458ae into main Jul 28, 2026
27 checks passed
@trunk-io
trunk-io Bot deleted the claude/dreamy-bardeen-0efkxi-memory-pool-cleanup branch July 28, 2026 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants