[multiverse RPC]: add better universe root node cache #1169
+1,131
−490
Conversation
This file contains 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 commit adds a completely new cache that is tailor made for the queries the syncer makes (ascending, without any amounts by ID). This additional cache has all roots (without details), so the existing cache will only be needed for custom queries that aren't coming from the syncer. We should be able to down-size the existing cache quite a bit, as it should only serve manual requests or requests coming from an UI. We'll make the size configurable in an upcoming commit.
Pull Request Test Coverage Report for Build 11666780661Details
💛 - Coveralls |
Roasbeef
reviewed
Nov 4, 2024
| // remainder. And the way the cache query is built (query, if not found, | ||
| // acquire lock, query again), we always make two queries for each page. | ||
| numMisses := ((numAssets / pageSize) + 1) * 2 | ||
| require.EqualValues(t, 0, multiverse.rootNodeCache.hit.Load()) |
There was a problem hiding this comment.
Nice usage of the internal stat counter here to test the his/misses!
| // paging through them. This map never needs to be cleared, as universe | ||
| // roots are only added (new issuance or transfer events) or modified | ||
| // (re-issuance into grouped asset or new transfer) but never removed. | ||
| // Therefore, when the cache needs to be wiped, we only need to clear |
There was a problem hiding this comment.
but never removed.
It's not exposed over the RPC interface rn, but they can be removed (see the DeleteUniverse method).
tapdb/multiverse_cache.go
Outdated
| return &syncerRootNodeCache{ | ||
| universeRoots: make( | ||
| map[universe.IdentifierKey]universe.Root, | ||
| numCachedProofs, |
There was a problem hiding this comment.
I think we can pre-size this to a larger size, as rn it's set to 50k, but we have ~175k assets on mainnet rn.
|
|
||
| // If we have the write lock already, no need to fetch it. | ||
| if !haveWriteLock { | ||
| r.RLock() |
There was a problem hiding this comment.
This grabs the read lock, but seems the intention is to grab the write lock.
| limit := q.Limit | ||
|
|
||
| // Is the page valid? | ||
| if offset < 0 || offset >= int32(len(r.universeRoots)) || limit <= 0 { |
| r.universeKeyList = append(r.universeKeyList, root.ID.Key()) | ||
| r.universeRoots[root.ID.Key()] = root | ||
|
|
||
| // To make sue we can easily add and remove entries, we sort the |
There was a problem hiding this comment.
Can pull into a helper method, as used above verbatim.
| // | ||
| // NOTE: This method must be called while holding the syncer cache lock. | ||
| func (r *syncerRootNodeCache) remove(key universe.IdentifierKey) { | ||
| idx := sort.Search(len(r.universeKeyList), func(i int) bool { |
There was a problem hiding this comment.
Nice usage of sort.Search here 👍
| }) | ||
| if idx < len(r.universeKeyList) && r.universeKeyList[idx] == key { | ||
| // Remove the entry from the list. | ||
| r.universeKeyList = append( |
There was a problem hiding this comment.
Can use the slices package here: https://pkg.go.dev/slices#Delete
| // caches that exist. | ||
| // | ||
| //nolint:lll | ||
| type MultiverseCacheConfig struct { |
| // multiverse cache. | ||
| func DefaultMultiverseCacheConfig() MultiverseCacheConfig { | ||
| return MultiverseCacheConfig{ | ||
| ProofsPerUniverse: 5, |
There was a problem hiding this comment.
Temp for this commit?
We should inherit the defaults defined in the existing package here.
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.
Fixes #880.
Opening as draft, mainly looking for concept ACK.
@Roasbeef the idea here is the following:
sort=ascendingandWithAmountsById=false, which allows us to optimize the new cache and keep all root nodes in memory for those queriesWithAmountsById=true, which allows us to keep a much smaller cacheTODOs: