Release query-engine memory back to the OS#1171
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds self-reaping for finished query threads, revises ChangesThread lifecycle and memory tuning
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agents/evolution/QueryEvolutionProcessor.cc`:
- Line 41: The worker cleanup in QueryEvolutionProcessor only happens during
run_command() and after workers enqueue their finished ids, so finished
StoppableThread entries can stay joinable in query_threads while idle. Update
QueryEvolutionProcessor to trigger reap_finished_threads() from a non-worker
path as well, and make sure the finished-id handling around the queue in the
worker-completion flow still feeds that cleanup. Add a matching *_test.cc case
under src/tests/cpp that covers the burst-then-idle scenario and verifies the
threads are reaped without waiting for another command.
In `@src/agents/query_engine/PatternMatchingQueryProcessor.cc`:
- Line 64: Finished workers are only reaped before spawning in run_command(),
while the completion path around the finished-id queue just enqueues and leaves
joinable StoppableThread entries in query_threads until another command arrives.
Add a non-worker reaping trigger so completed threads are joined as soon as they
finish, and update the relevant cleanup path in PatternMatchingQueryProcessor to
remove them without waiting for the next command. Also add a matching *_test.cc
case that exercises the burst-then-idle scenario to verify query_threads returns
to baseline after workers finish.
In `@src/main/bus_node.cc`:
- Around line 32-45: The glibc memory-tuning block in bus_node.cc overrides the
operator’s trim setting by always calling mallopt for the trim threshold. Update
the startup logic around the existing getenv("MALLOC_ARENA_MAX") /
mallopt(M_ARENA_MAX, 4) handling so the M_TRIM_THRESHOLD setting is also skipped
when an explicit allocator trim tunable is present, or otherwise clearly
document that BusNode initialization intentionally overrides allocator
configuration. Use the existing bus-node initialization section and the
mallopt(M_TRIM_THRESHOLD, ...) call as the main place to apply the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 831c284b-ad75-494b-a7bb-66f8e7a157fc
📒 Files selected for processing (5)
src/agents/evolution/QueryEvolutionProcessor.ccsrc/agents/evolution/QueryEvolutionProcessor.hsrc/agents/query_engine/PatternMatchingQueryProcessor.ccsrc/agents/query_engine/PatternMatchingQueryProcessor.hsrc/main/bus_node.cc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/tests/cpp/stoppable_thread_test.cc (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAdd one processor-level regression for the actual cleanup path.
This mirrors the map/self-detach pattern, but it won’t catch broken
PatternMatchingQueryProcessororQueryEvolutionProcessorwiring. Add a focused burst-then-idle test around at least one real processor path soquery_threadsdrains without a follow-up command.As per coding guidelines, production behavior changes require matching tests. As per path instructions, tests should cover real behavior, not trivial coverage.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/cpp/stoppable_thread_test.cc` around lines 14 - 19, The current StoppableThread test only covers the generic self-reap pattern and does not exercise the real cleanup path in PatternMatchingQueryProcessor or QueryEvolutionProcessor. Add a burst-then-idle regression test against at least one of those processor classes so the actual query_threads lifecycle is covered end-to-end, verifying that the worker map drains to empty after a burst without requiring any extra command or external reap trigger. Use the existing processor setup/teardown and the relevant query execution entry point to drive real work and assert the cleanup behavior in the processor-specific test.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commons/StoppableThread.cc`:
- Around line 34-35: The early return in StoppableThread::stop should not reuse
stop_flag as a “already cleaned up” signal, because that can skip the
join/detach/delete path for the thread_object. Update the stop() and destructor
flow to separate “stop requested” from thread cleanup state, and make sure
joinable threads are always reaped exactly once even after stop(false), later
stop() calls, or self-reap detach() paths.
---
Nitpick comments:
In `@src/tests/cpp/stoppable_thread_test.cc`:
- Around line 14-19: The current StoppableThread test only covers the generic
self-reap pattern and does not exercise the real cleanup path in
PatternMatchingQueryProcessor or QueryEvolutionProcessor. Add a burst-then-idle
regression test against at least one of those processor classes so the actual
query_threads lifecycle is covered end-to-end, verifying that the worker map
drains to empty after a burst without requiring any extra command or external
reap trigger. Use the existing processor setup/teardown and the relevant query
execution entry point to drive real work and assert the cleanup behavior in the
processor-specific test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 60c544c8-f921-4594-87bd-ca2b47db013c
📒 Files selected for processing (9)
src/agents/evolution/QueryEvolutionProcessor.ccsrc/agents/evolution/QueryEvolutionProcessor.hsrc/agents/query_engine/PatternMatchingQueryProcessor.ccsrc/agents/query_engine/PatternMatchingQueryProcessor.hsrc/commons/StoppableThread.ccsrc/commons/StoppableThread.hsrc/main/bus_node.ccsrc/tests/cpp/BUILDsrc/tests/cpp/stoppable_thread_test.cc
💤 Files with no reviewable changes (2)
- src/agents/evolution/QueryEvolutionProcessor.h
- src/agents/query_engine/PatternMatchingQueryProcessor.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/bus_node.cc
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commons/ThreadSafeHeap.h (1)
86-94: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReserve output capacity before draining the snapshot.
copy.size()is known before the loop, so reserve the destination vector to avoid repeated reallocations when snapshotting large candidate heaps.Proposed change
{ lock_guard<mutex> semaphore(this->api_mutex); copy = this->queue; } + output.reserve(output.size() + copy.size()); while (!copy.empty()) { output.push_back(copy.top().element); copy.pop(); }As per path instructions, “MEMORY & PERFORMANCE (high priority): Flag unnecessary copies of large objects ... missing reserve() before repeated push_back/emplace_back.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commons/ThreadSafeHeap.h` around lines 86 - 94, The snapshot-draining logic in ThreadSafeHeap::snapshot (the block that copies this->queue into the local priority_queue copy and then appends to output) should reserve the destination vector upfront. Use the known size of copy before the while loop to call reserve on output, then keep the push_back/pop draining as-is to avoid repeated reallocations for large heaps.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/commons/ThreadSafeHeap.h`:
- Around line 86-94: The snapshot-draining logic in ThreadSafeHeap::snapshot
(the block that copies this->queue into the local priority_queue copy and then
appends to output) should reserve the destination vector upfront. Use the known
size of copy before the while loop to call reserve on output, then keep the
push_back/pop draining as-is to avoid repeated reallocations for large heaps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 57e4dc8d-dc8c-467a-8bbb-f2bd7034580d
📒 Files selected for processing (1)
src/commons/ThreadSafeHeap.h
andre-senna
approved these changes
Jul 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
query-enginebus node kept growing in memory duringrun_toy.shand stayedaround ~15GB even after the workload finished. The node is a long-lived service, so
whatever it accumulated was never given back.
Root causes:
allocations (HandleSets of tens of thousands of atoms + QueryAnswers) and frees
them, but with up to
8 * nprocmalloc arenas and nomalloc_trim, glibc parkedthose pages in per-arena free lists. RSS stayed pinned at the peak.
PatternMatchingQueryProcessorandQueryEvolutionProcessorboth left a// TODO add a call to remove_query_thread,so every completed query thread stayed joinable and held its stack/arena.
Changes
bus_node.cc: cap glibc arenas (M_ARENA_MAX=4) and lower the trim threshold(
M_TRIM_THRESHOLD) so freed pages are returned to the OS. Both are only appliedwhen the operator has not set the corresponding
MALLOC_ARENA_MAX/MALLOC_TRIM_THRESHOLD_env var, so explicit allocator tuning is respected.PatternMatchingQueryProcessor/QueryEvolutionProcessor: callmalloc_trim(0)after a query finishes (once its tree and answers are freed), and have each worker
self-reap: a thread cannot join itself, so when it finishes it detaches and
removes its own entry from
query_threads. This returns to baseline immediately,including the burst-then-idle case (no dependency on a later command).
StoppableThread: adddetach()and separate the stop state from threadcleanup.
stop_flagnow only means "stop requested" (whatstopped()reports);reaping claims
thread_objectexactly once (claim-and-null under the lock, join/detachoutside it). So
stop()/detach()are idempotent and null-safe,stop(false)canrequest a stop and still have the thread reaped by a later
stop()/detach()/destructor,and a thread can release itself without a later join.
All allocator calls are guarded with
#if defined(__GLIBC__).Tests
stoppable_thread_test.cc: covers the self-reap mechanism — a burst of workersthat each detach + erase themselves drains to empty without any external reaping
trigger,
stop()afterdetach()is a safe no-op, andstop(false)(request-only)followed by a later
stop()still reaps the thread.Result (query-engine container during
run_toy.sh)Query behavior is unchanged; only memory lifecycle is affected.