Skip to content

Conversation

@C-Pro
Copy link
Owner

@C-Pro C-Pro commented Nov 29, 2025

Add a separate KVCache with slice based radix implementation (compared to map based wrapper that is KV).
The idea is to cut on memory consumption as well as search time on the trie node level (small maps access is not faster than small slices, and max map size on node level is 256).
And by making it a separate cache size we are cutting down on extra mutex in the wrapped cache.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces a new KVCache implementation using a slice-based radix trie instead of the existing map-based wrapper (KV). The primary goals are to reduce memory consumption and improve search performance at the trie node level, while eliminating the extra mutex overhead from the wrapped cache approach.

Key Changes

  • Added KVCache with slice-based radix trie implementation for memory efficiency
  • Updated RingBuffer.All() to use exclusive lock instead of read lock
  • Enhanced benchmarks with new test cases and more realistic composite key patterns

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
kv_cache.go New slice-based radix trie cache implementation with terminal nodes and freelist for value reuse
kv_cache_test.go Comprehensive test suite including unit tests, fuzz tests, and concurrency tests for KVCache
bench_test.go Added KVCache to benchmark suite and enhanced test data generation with composite keys
ring.go Changed All() iterator from RLock to exclusive Lock

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Nov 29, 2025

Codecov Report

❌ Patch coverage is 95.02488% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.98%. Comparing base (e5b429f) to head (6be9a39).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
kv_cache.go 95.02% 15 Missing and 5 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main      #17      +/-   ##
===========================================
- Coverage   100.00%   97.98%   -2.02%     
===========================================
  Files            7        8       +1     
  Lines          790     1190     +400     
===========================================
+ Hits           790     1166     +376     
- Misses           0       18      +18     
- Partials         0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@C-Pro C-Pro requested review from arthhhhh23 and egregors November 29, 2025 15:22
Copy link
Collaborator

@egregors egregors left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Image LGTM

@C-Pro
Copy link
Owner Author

C-Pro commented Nov 30, 2025

LGTM

I trust you have diligently checked every line with a looking glass 🕵️

func (kv *KVCache[K, V]) get(key K) (V, bool) {
node := kv.trie
keyBytes := []byte(key)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add same logic as insert

	if len(keyBytes) == 0 {
		if node.terminal {
			return kv.values[node.valueIndex], true
		}
		return kv.zero, false
	}

Copy link
Owner Author

@C-Pro C-Pro Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this instead:

	if node.terminal {
		return kv.values[node.valueIndex], true
	}

	return kv.zero, false

@C-Pro C-Pro merged commit 6a93e00 into main Dec 4, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants