Refactor cache implementations #3239
Merged
Conversation
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
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
…avalanchego into refactor-cache-implementations
StephenButtolph
commented
May 1, 2025
Contributor
Author
There was a problem hiding this comment.
Removes some stutters
Comment on lines
-43
to
-55
|
|
||
| cache.Size = 2 | ||
|
|
||
| expectedValue3 := &evictable[ids.ID]{id: ids.ID{2}} | ||
| returnedValue = cache.Deduplicate(expectedValue3) | ||
| require.Equal(expectedValue2, returnedValue) | ||
| require.Equal(1, expectedValue1.evicted) | ||
| require.Zero(expectedValue2.evicted) | ||
|
|
||
| cache.Flush() | ||
| require.Equal(1, expectedValue1.evicted) | ||
| require.Equal(1, expectedValue2.evicted) | ||
| require.Zero(expectedValue3.evicted) |
Contributor
Author
There was a problem hiding this comment.
The size is no longer mutable.
joshua-kim
approved these changes
May 1, 2025
| {Size: 2, Func: Eviction}, | ||
| } | ||
|
|
||
| func TestBasic(t *testing.T, cache cache.Cacher[ids.ID, int64]) { |
Contributor
There was a problem hiding this comment.
Why are these renamed? Is this because we felt the cachetest.Test* is stuttering?
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors cache implementations by moving the LRU cache into its own package and deprecating legacy cache code.
- Migrates cache.LRU usage to the new lru package API (lru.NewCache and lru.NewSizedCache).
- Removes legacy cache files and updates corresponding tests.
Reviewed Changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| snow/engine/avalanche/bootstrap/queue/state.go | Updates cache reference from lru implementations. |
| snow/engine/avalanche/bootstrap/bootstrapper.go | Replaces legacy cache with new lru.NewCache usage. |
| network/p2p/gossip/gossip.go | Converts cache.LRU usage to lru.NewCache in gossip implementation. |
| network/p2p/acp118/handler_test.go | Updates test cache instantiation to lru.NewCache. |
| database/linkeddb/linkeddb.go | Migrates node cache creation to use lru.NewCache. |
| cache/unique_cache.go | Removed unique cache implementation. |
| cache/metercacher/cache_test.go | Refactored tests to use lru.NewCache and lru.NewSizedCache. |
| cache/lru/sized_cache_test.go | Updates sized cache tests with new API and renames test functions. |
| cache/lru/sized_cache.go | Refactors sized cache implementation and naming. |
| cache/lru/deduplicator_test.go | Adapts deduplicator tests to new naming conventions. |
| cache/lru/deduplicator.go | Adds new deduplicator implementation in the lru package. |
| cache/lru/cache_test.go | Introduces tests for the new lru.Cache implementation. |
| cache/lru/cache.go | Implements the new lru.Cache as a replacement for legacy code. |
| cache/empty.go | Adds a clarifying comment to the Empty cache implementation. |
| cache/cachetest/cacher.go | Updates test function names to match the new lru API. |
| cache/cache.go | Removes legacy Deduplicator and Evictable definitions. |
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (2)
cache/lru/sized_cache_test.go:4
- [nitpick] Changing the test package from 'cache_test' to 'lru' may expose unexported identifiers. If white-box testing is not required, consider using 'lru_test' to maintain proper test isolation.
package lru
cache/cachetest/cacher.go:26
- [nitpick] Ensure that renaming the test function from 'TestBasic' to 'Basic' is consistent with your project's test naming conventions to avoid confusion across the codebase.
{Size: 1, Func: Basic},
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.
Why this should be merged
The
cachepackage is currently a bit odd - as there is ametercachersub-package (similar to thedatabaseimplementations). Butlruis implemented in the top levelcachepackage.This moves the
lruimplementations out of thecachepackage into their own package (to align with the database structure)How this works
How this was tested
Existing tests