Replace O(n) deque scans with O(log n) ordering in ReActAgent's bounded slot cache - #4
Draft
java-dependency-upgrade-fixer[bot] wants to merge 1 commit into
Conversation
Owner
|
Accepted for the JAIPilot evidence campaign — additional success 7/10 after the ShardingSphere anchor. Independent review:
Boundary: this counts the proved cache-bookkeeping complexity reduction. It does not claim repository-wide serialization or that the companion repairs any pre-existing publish/trim ordering outside the measured changed path. |
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
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.
What changed
ReActAgent's
stateCache/permissionEngineCachebound (added in commit 73bb1cb, PR agentscope-ai#2432 mirror) tracked slot recency with aConcurrentLinkedDeque<String>, usingdeque.remove(slot)to promote an accessed slot anddeque.size()in awhileloop to decide when to evict. Both operations are linear scans; with the cache sized up to 1,000 entries, every single call toactivateSlotForContext/getAgentStatepaid up to ~1,000 String comparisons twice (once to promote, repeatedly to check size while trimming).This change replaces the deque with:
ConcurrentHashMap<String, Long> slotLastAccess— each slot's most recent access-sequence number.ConcurrentSkipListMap<Long, String> slotOrder— sequence number -> slot, giving an ordered, O(log n) structure for "oldest first" eviction.AtomicLong slotAccessSequence— monotonic sequence generator.AtomicInteger trackedSlotCount— O(1) bound check instead of scanning for size.Promotion (
recordSlotAccess) and eviction (trimCaches) are now O(log n) instead of O(n), where n is bounded at 1,000 (MAX_CACHED_SLOTS).Correctness fix found during this pass
An initial version of this change decremented
trackedSlotCountwhenevertrimCachespolled the globally-oldest sequence entry, without checking whether that entry was still the slot's authoritative position. Under concurrency, a slot being promoted byrecordSlotAccesscan transiently leave its old sequence entry inslotOrdera moment before it's cleaned up; iftrimCachespolls that stale entry first, unconditionally evicting the slot's cache entries and decrementing the counter would (a) evict a slot that was just freshly accessed, and (b) permanently under-counttrackedSlotCount, letting the cache silently grow past the 1,000-slot bound over time under sustained contention.The fix makes the eviction conditional:
slotLastAccess.remove(slot, oldest.getKey())only succeeds (and only then do we remove fromstateCache/permissionEngineCacheand decrement the counter) when the polled sequence number is still that slot's current position. A stale entry is discarded fromslotOrder(harmless — it was garbage) but does not shrink the tracked count, and the eviction loop continues until it finds a real, still-current oldest entry. This was verified empirically with a 32-thread, ~96k-operation stress test racing a small pool of "hot" slots against a stream of brand-new slots (maximizing promotion/eviction races); the 1,000-slot bound held exactly across repeated runs, both forstateCacheandpermissionEngineCache.Preserved behavior
MAX_CACHED_SLOTS) is unchanged.AgentStateandPermissionEnginetogether; a permission-engine entry can never outlive its paired state entry.AgentStateStorereload behavior, legacy-state loading, and loader exception handling are untouched (no changes toloadOrCreateAgentStateForSlot).Tests
Added
ReActAgentSlotCacheEvictionTest(5 tests) locking: the 1,000-slot bound, LRU-refresh protection against eviction, paired state/permission-engine removal, concurrent-activation boundedness, and the no-storegetAgentStatebound. These pass unmodified against both the original PR-head implementation and the final candidate.Limitations
ConcurrentSkipListMap.size()is also not O(1) per its Javadoc, which is why an explicitAtomicIntegercounter is used instead of calling.size().Generated by JAIPilot Cloud for #3 from Anthropic session
sesn_01V7Z4eE6MSfGsevGGZgXGk3.