mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-26 15:40:18 +00:00
perf: use Arc in embedding cache to avoid clones on miss path (#1438)
* docs: add comments explaining CLI_ENABLED=false in service templates (#990) Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd) to prevent blocking on stdin when running as a background service. Closes #990 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * perf: use Arc<Vec<f32>> in embedding cache to avoid clones on miss path (#1429) Store embeddings as Arc<Vec<f32>> internally so that cache insertions share the allocation with the return value via Arc::clone instead of cloning the entire float vector (6-12 KB per embedding). - embed() miss path: Arc::try_unwrap avoids a clone when returning (the cache holds one Arc ref, the return path holds the other; try_unwrap succeeds when the thundering-herd path doesn't fire) - embed_batch() miss path: cache first via Arc::clone, then try_unwrap for results — embeddings skipped due to capacity limits are returned without any clone - Hit path still clones (trait returns Vec<f32>); a future trait change to Arc<Vec<f32>> could eliminate this too Closes #1429 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * style: fix formatting in embedding_cache.rs Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: address PR review — correct doc comment and remove dead try_unwrap - Reword CacheEntry doc comment to accurately reflect that hit/miss paths still clone into a fresh Vec<f32> for callers; Arc sharing only helps in embed_batch when embeddings are skipped from caching - Remove Arc::try_unwrap in embed() which could never succeed (cache always holds an Arc ref, so refcount >= 2) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: revert embed() to plain Vec, keep Arc only in embed_batch() In embed(), Arc adds overhead (allocation + refcount) without saving any clones — the original pattern (clone for cache, return by move) was already optimal. Arc only helps in embed_batch() where capacity-skipped embeddings can be returned via try_unwrap. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix: move clone+Arc::new outside mutex in embed() Clone the embedding and wrap in Arc before acquiring the lock so the mutex is held only for the HashMap insert, not during the O(n) copy. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * refactor: drop Arc, use cache-then-move pattern instead Arc was the wrong abstraction — the trait returns Vec<f32>, so Arc can't avoid clones on return paths. Instead: - embed(): skip clone in thundering-herd case (just touch timestamp) - embed_batch(): cache first (clone only cacheable subset), then move originals into results (zero-copy). For N misses with K cacheable: old = 2N clones, new = K clones. - CacheEntry reverted to plain Vec<f32>, no Arc overhead Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.6
parent
cac6f4013c
commit
6b0f84bbe0
@@ -183,8 +183,8 @@ impl EmbeddingProvider for CachedEmbeddingProvider {
|
||||
{
|
||||
let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner());
|
||||
if let Some(entry) = guard.get_mut(&key) {
|
||||
// Key already present (thundering herd) — just update, no eviction needed.
|
||||
entry.embedding = embedding.clone();
|
||||
// Thundering herd — another caller already cached it.
|
||||
// Just touch timestamp; skip the clone.
|
||||
entry.last_accessed = Instant::now();
|
||||
} else {
|
||||
Self::evict_lru(&mut guard, self.config.max_entries);
|
||||
@@ -260,15 +260,10 @@ impl EmbeddingProvider for CachedEmbeddingProvider {
|
||||
"embedding batch: partial cache"
|
||||
);
|
||||
|
||||
// Assemble results first (all misses, regardless of cache capacity).
|
||||
for (orig_idx, emb) in miss_indices.iter().copied().zip(&new_embeddings) {
|
||||
results[orig_idx] = Some(emb.clone());
|
||||
}
|
||||
|
||||
// Cache the new embeddings, respecting max_entries.
|
||||
// Cache FIRST (clone only the cacheable subset), then move originals
|
||||
// into results. This avoids cloning capacity-skipped embeddings entirely.
|
||||
{
|
||||
let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner());
|
||||
// When misses exceed capacity, clear and only cache the tail.
|
||||
let cacheable = miss_indices.len().min(self.config.max_entries);
|
||||
let skip = miss_indices.len() - cacheable;
|
||||
let need_to_evict = (guard.len() + cacheable).saturating_sub(self.config.max_entries);
|
||||
@@ -287,6 +282,11 @@ impl EmbeddingProvider for CachedEmbeddingProvider {
|
||||
}
|
||||
}
|
||||
|
||||
// Move originals into results (zero-copy for all, including cached ones).
|
||||
for (orig_idx, emb) in miss_indices.iter().copied().zip(new_embeddings) {
|
||||
results[orig_idx] = Some(emb);
|
||||
}
|
||||
|
||||
results
|
||||
.into_iter()
|
||||
.enumerate()
|
||||
|
||||
Reference in New Issue
Block a user