-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CBG-4032: memory based rev cache implementation #7040
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall approach seems ok, a few questions/suggestions.
func (rc *LRURevisionCache) revCacheMemoryBasedEviction() { | ||
// if memory capacity is not set, don't check for eviction this way | ||
if rc.memoryCapacity > 0 && rc.memoryBytes.Value() > rc.memoryCapacity { | ||
rc.performEviction() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only going to evict a single value, and there's no guarantee that the evicted value is large enough to offset the incoming value - could end up in a situation where 100 20MB documents trigger the eviction of 100 1k documents, and leave the cache significantly above the memory target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I follow. This function is to determine if we need eviction based upon if we have a max size set & the current memory count is above the threshold.
If true it will call performEviction()
which acquires the lock and runs a loop for eviction till the memory stat is below the threshold.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my mistake - misread performEviction as 'if' instead of 'for'.
db/revision_cache_lru.go
Outdated
@@ -359,6 +388,7 @@ func (value *revCacheValue) asDocumentRevision(delta *RevisionDelta) (DocumentRe | |||
Attachments: value.attachments.ShallowCopy(), // Avoid caller mutating the stored attachments | |||
Deleted: value.deleted, | |||
Removed: value.removed, | |||
MemoryBytes: value.itemBytes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asDocumentRevision is only intended to copy immutable properties (and so doesn't require callers to hold value.lock.Lock). With the updateDelta handling I don't think itemBytes can be treated as immutable, but it's also important from a performance perspective to not hold the lock.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option might be to separate the delta and non-delta byte storage on revCacheValue, and compute the sum for DocumentRevision.MemoryBytes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot. I have ended up removing this all together as I can work around this by uisng the getItemBytes()
function that acquired read lock to incr/decr the stat wherever necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still feel like there is a lot of unnecessary locking of itemBytes in the case where delta sync is not being used (and so itemBytes is immutable), but I'm fine coming back to that based on analysis of the perf results.
* CBG-4032: memory based rev cache implementation * make test pass with default collection + missing return param * more issues with default collection test * touch ups on comment + remove unused function * updated for review * further tidy up * lint fix after refector test * fixes from rebase * updates based off review + some more refactoring
* CBG-4023: Removal unmarshalled body on the rev cache (#7030) * CBG-4135: new stat for rev cache capacity (#7049) * CBG-4135: new stat for rev cache capacity * updated based on review * fix linter * lint again * CBG-4032: memory based rev cache implementation (#7040) * CBG-4032: memory based rev cache implementation * make test pass with default collection + missing return param * more issues with default collection test * touch ups on comment + remove unused function * updated for review * further tidy up * lint fix after refector test * fixes from rebase * updates based off review + some more refactoring * CBG-4134: link rev cache memory limit config option to rev cache (#7084) * CBG-4134: link rev cache memory limit config option to rev cache * failing tests * address commnets * fix yaml lint * fix failing test + mistake in docs Signed-off-by: Gregory Newman-Smith <[email protected]> --------- Signed-off-by: Gregory Newman-Smith <[email protected]> * CBG-4234: clean up rev cache work (#7113) * CBG-4234: clean up rev cache work * new test * CBG-4277: Remove unused totalBytesForHistory from getHistory (#7137) --------- Signed-off-by: Gregory Newman-Smith <[email protected]> Co-authored-by: Gregory Newman-Smith <[email protected]>
* CBG-4023: Removal unmarshalled body on the rev cache (#7030) * CBG-4135: new stat for rev cache capacity (#7049) * CBG-4135: new stat for rev cache capacity * updated based on review * fix linter * lint again * CBG-4032: memory based rev cache implementation (#7040) * CBG-4032: memory based rev cache implementation * make test pass with default collection + missing return param * more issues with default collection test * touch ups on comment + remove unused function * updated for review * further tidy up * lint fix after refector test * fixes from rebase * updates based off review + some more refactoring * CBG-4134: link rev cache memory limit config option to rev cache (#7084) * CBG-4134: link rev cache memory limit config option to rev cache * failing tests * address commnets * fix yaml lint * fix failing test + mistake in docs Signed-off-by: Gregory Newman-Smith <[email protected]> --------- Signed-off-by: Gregory Newman-Smith <[email protected]> * CBG-4234: clean up rev cache work (#7113) * CBG-4234: clean up rev cache work * new test * CBG-4277: Remove unused totalBytesForHistory from getHistory (#7137) --------- Signed-off-by: Gregory Newman-Smith <[email protected]> Co-authored-by: Gregory Newman-Smith <[email protected]>
CBG-4032
Implementation of memory bounded rev cache.
Summary of changes:
RevisionCacheOptions
to link up to RevCache code for the overall memory limit to be configured on. Temporarily this way, CBG-4134 will finish this side of the work.NewShardedLRURevisionCache
Pre-review checklist
fmt.Print
,log.Print
, ...)base.UD(docID)
,base.MD(dbName)
)docs/api
Integration Tests
GSI=true,xattrs=true
https://jenkins.sgwdev.com/job/SyncGateway-Integration/2664/