Optimize handles clearing on session / token closing#851
Optimize handles clearing on session / token closing#851bukka wants to merge 1 commit intosofthsm:mainfrom
Conversation
📝 WalkthroughWalkthroughThe handle manager introduces three secondary indexes— Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/handle_mgr/HandleManager.cpp (1)
161-165: Avoidoperator[]in cleanup-only accesses to the secondary indexes.These sites silently create empty buckets when the index is already out of sync. The risky case is Line 202: a missing
slotSessionCountentry becomes0, and the zero-path below clears the whole slot. Preferfind()/erase()here so an invariant miss stays visible instead of mutating state during cleanup.Also applies to: 182-182, 194-194, 202-204, 257-257
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/handle_mgr/HandleManager.cpp` around lines 161 - 165, The cleanup code uses operator[] on secondary-index maps (e.g., sessionObjectHandles, slotHandles, slotSessionCount) which can silently create empty buckets; change these accesses to use find() and conditional erase only when the key exists (use iterator from find(), then call erase(it) or container.erase(iterator->second.find(hObject)) as appropriate) so missing entries do not get created during cleanup—update all occurrences including the removal from sessionObjectHandles/slotHandles and the slotSessionCount decrement/clear paths to use find() checks before modifying or erasing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/handle_mgr/HandleManager.cpp`:
- Around line 161-165: The cleanup code uses operator[] on secondary-index maps
(e.g., sessionObjectHandles, slotHandles, slotSessionCount) which can silently
create empty buckets; change these accesses to use find() and conditional erase
only when the key exists (use iterator from find(), then call erase(it) or
container.erase(iterator->second.find(hObject)) as appropriate) so missing
entries do not get created during cleanup—update all occurrences including the
removal from sessionObjectHandles/slotHandles and the slotSessionCount
decrement/clear paths to use find() checks before modifying or erasing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b1d9522-e39c-4c70-ba1f-5ed1e9178c3b
📒 Files selected for processing (2)
src/lib/handle_mgr/HandleManager.cppsrc/lib/handle_mgr/HandleManager.h
Currently the full map scan is done on each clearing. This can result in significant CPU usage when large number of objects are used. The main problem is that full scan is done when any session is closed so if application has large number in one session, they get scanned even though there is no need for that.
This PR introduces additional maps that track object per session / slot as well as slot session count.
Summary by CodeRabbit