perf: reduce agent latency across compression, state, and turn hot paths - #54710
Cemontero0417 wants to merge 1 commit into
Conversation
WAL checkpoint frequency 50→10 writes to keep SQLite query times fast as sessions grow. FTS index backfill deferred to a daemon thread for databases ≥500 messages, eliminating multi-second startup freezes after schema upgrades. Context compression pre-warms summary generation in a background thread when context reaches 80% of threshold, so compress() joins a nearly-done future instead of blocking cold for 2–10s. Tool-output pruning switches from an O(n) upfront deep-copy to a lazy shallow-list copy. Memory manager replaces per-session ThreadPoolExecutors with a single shared pool (max_workers=4), dropping thread count from N×1 to 4 under concurrent gateway load while preserving per-session write ordering via a serializer lock. Nous rate guard skips the atomic file write when existing state already covers the same reset window (< 5s delta), cutting filesystem I/O during 429 storms. Skills manifest walk cached for 30s so repeated snapshot validations don't stat every skill file. Skill scaffolding parsing memoized with lru_cache(4) to avoid re-parsing the same message in prefetch and sync paths. Iteration budget read-only properties drop their lock (GIL guarantees atomic int reads). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: COMMENT — high surface area, human review recommended
This PR touches 8+ files across core modules (context_compressor, conversation_loop, memory_manager, prompt_builder, hermes_state, nous_rate_guard, iteration_budget) with multiple independent optimizations. Each change is individually sound, but the breadth warrants a human pass before merge.
Individual Changes (all appear correct):
- Pre-warm summary generation (context_compressor): background thread starts summary at 80% threshold. Fingerprint-based cache avoids redundant work. 180s timeout on join.
- Shallow list copy in _prune_old_tool_results: O(k) instead of O(n) deep copy — correct since dicts are copied on write.
- IterationBudget lock removal: plain int read is atomic under CPython GIL — correct.
- Global memory sync executor: shared ThreadPoolExecutor(max_workers=4) replaces per-session pools. Per-session write ordering via _write_serializer lock. flush_pending uses concurrent.futures.wait.
- LRU cache on _strip_skill_scaffolding: memoizes repeated calls with same text.
- Manifest cache TTL (30s): avoids repeated O(skill_files) stat walks on every gateway request.
- Rate guard dedup: skips filesystem write if existing state file covers same reset time within 5s.
- FTS backfill deferral: background thread for large DBs (>=500 messages), inline for small. Properly handles trigger repair path.
- WAL checkpoint frequency: 50 -> 10 writes between checkpoints.
Suggestion
Consider splitting this into 2-3 smaller PRs (e.g., compression prewarm + memory executor refactor, FTS backfill + WAL checkpoint, iteration_budget + rate_guard). Easier to bisect if any regression surfaces.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for investigating several real hot paths. Current main still has the synchronous FTS rebuild (hermes_state.py:1672-1678) and 50-write checkpoint cadence (hermes_state.py:933-934), but this bundle has correctness regressions that need rework.
Problems
agent/context_compressor.py:973-975runs_generate_summaryconcurrently without a session-generation fence. Current reset/end paths clear mutable compression state (agent/context_compressor.py:726-785), so a running prewarm can complete into a later session.agent/memory_manager.py:61-64changes background memory work to stdlibThreadPoolExecutor. Current main deliberately usesDaemonThreadPoolExecutor(agent/memory_manager.py:654-662); its implementation documents why standard executor workers can block interpreter exit (tools/daemon_pool.py:3-16).hermes_state.py:1424-1437starts FTS backfill after initialization returns. That permits searches while historical rows are still absent, unlike the current synchronous rebuild athermes_state.py:1672-1678.
Suggested changes
- Fence prewarm results by session/window generation and test reset while work is running.
- Retain daemon-safe shared workers and add shutdown coverage for a wedged provider.
- Preserve immediate FTS-search correctness, with a >=500-message migration regression test.
This is an automated hermes-sweeper review.
| self._prewarm_future.cancel() | ||
| self._prewarm_fingerprint = fp | ||
| focus = self._derive_auto_focus_topic(msgs_copy) | ||
| self._prewarm_future = self._prewarm_executor.submit( |
There was a problem hiding this comment.
This runs _generate_summary against mutable compressor state without a session-generation fence. on_session_reset() and on_session_end() reset that state, but a running Future cannot be cancelled; tag the work with a session/window generation and discard a late result after reset.
| global _GLOBAL_MEM_SYNC_EXECUTOR | ||
| if _GLOBAL_MEM_SYNC_EXECUTOR is not None: | ||
| return _GLOBAL_MEM_SYNC_EXECUTOR | ||
| with _GLOBAL_MEM_SYNC_EXECUTOR_LOCK: |
There was a problem hiding this comment.
Please retain daemon-safe workers here. Current main uses DaemonThreadPoolExecutor specifically because stdlib ThreadPoolExecutor workers are joined by the interpreter's atexit hook; this can reintroduce hangs when a provider is blocked on I/O.
| _msg_count = 0 | ||
| if _msg_count >= 500: | ||
| # Large database — defer to background so startup doesn't freeze. | ||
| _t = threading.Thread( |
There was a problem hiding this comment.
Starting the backfill here returns a SessionDB with FTS enabled before old rows are indexed. An immediate search can omit historical messages; preserve a readiness/fallback contract and add a >=500-message migration test that searches immediately after open.
|
Closing after a hunk-by-hunk adjudication against current main — thank you @chris-montero for a wide-ranging and genuinely perf-literate PR. The verdict is that each piece is either superseded, deliberately designed differently on main, or unsafe as written; the full table lives in our triage notes, summary here:
Several of these ideas were independently proven right by where main ended up — the instinct was correct even where the implementation is now moot. If you want to pursue the lru_cache micro-win or a compute-only prewarm on top of #73017, fresh focused PRs against current main would be welcome. |
Summary
ThreadPoolExecutor(max_workers=1)instances with a shared pool (max_workers=4), cutting thread count from N×1 to 4 under concurrent gateway load while preserving per-session write orderinglru_cache(maxsize=4)to avoid re-parsing the same message across prefetch and sync pathsTest plan
🤖 Generated with Claude Code