CBG-4032: memory based rev cache implementation #7040

gregns1 · 2024-08-05T15:34:11Z

CBG-4032

Implementation of memory bounded rev cache.
Summary of changes:

Added stats for overall rev cache size in bytes
This is passed into the rev cache and atomically incremented/decremented as needed
Added a function on DocumentRev to calculate the document size based on what we agreed to be measured, channels, revisions and body in bytes.
This is called when adding to rev cache for new docs, and after a load from the bucket
There is also a version of this for revision deltas
Added a representation of a documents size in DocumentRevision and rev cache value, for purposes of passing up and down stack.
Create a config value in 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.
Left number based eviction where it is. Added a new function for memory based eviction that is called separately after the value is populated with its size + incremented the stat to reflect this.
Tests covering each operation on rev cache, eviction and delta caching. In addition have some test coverage of loading from the bucket.
For sharded revision cache memory limit, I have copied what we already fo for the number capacity (see NewShardedLRURevisionCache

Pre-review checklist

Removed debug logging (fmt.Print, log.Print, ...)
Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Integration Tests

GSI=true,xattrs=true https://jenkins.sgwdev.com/job/SyncGateway-Integration/2664/

adamcfraser

Overall approach seems ok, a few questions/suggestions.

base/stats.go

db/revtree.go

db/crud.go

db/revision_cache_lru.go

db/revision_cache_interface.go

db/revision_cache_lru.go

db/revision.go

db/revision_cache_interface.go

adamcfraser · 2024-08-20T21:44:05Z

db/revision_cache_lru.go

+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()


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.

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.

This was my mistake - misread performEviction as 'if' instead of 'for'.

adamcfraser · 2024-08-20T22:21:15Z

db/revision_cache_lru.go

@@ -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,


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.

One option might be to separate the delta and non-delta byte storage on revCacheValue, and compute the sum for DocumentRevision.MemoryBytes here.

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.

db/revision_cache_lru.go

adamcfraser

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]>

gregns1 force-pushed the CBG-4032 branch from 1fcfc03 to 94f1ab0 Compare August 5, 2024 16:04

gregns1 marked this pull request as ready for review August 6, 2024 09:45

gregns1 requested review from bbrks and adamcfraser August 6, 2024 09:45

gregns1 assigned bbrks and torcolvin Aug 6, 2024

adamcfraser assigned adamcfraser and unassigned torcolvin and bbrks Aug 6, 2024

adamcfraser requested changes Aug 6, 2024

View reviewed changes

adamcfraser requested changes Aug 20, 2024

View reviewed changes

gregns1 added 9 commits August 21, 2024 10:59

CBG-4032: memory based rev cache implementation

6a5206f

make test pass with default collection + missing return param

c31dfdd

more issues with default collection test

3eb9fa5

touch ups on comment + remove unused function

3166177

updated for review

326eced

further tidy up

4ca8499

lint fix after refector test

3ac53d0

fixes from rebase

c3192d3

updates based off review + some more refactoring

0cdbab9

gregns1 force-pushed the CBG-4032 branch from bf86fed to 0cdbab9 Compare August 21, 2024 14:10

gregns1 mentioned this pull request Aug 21, 2024

CBG-4134: link rev cache memory limit config option to rev cache #7084

Merged

5 tasks

adamcfraser approved these changes Aug 29, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Aug 29, 2024

adamcfraser merged commit 9a3f1b8 into main Aug 29, 2024
40 checks passed

adamcfraser deleted the CBG-4032 branch August 29, 2024 15:48

bbrks mentioned this pull request Sep 26, 2024

[3.2.1 Backport] CBG-4151: Memory-based rev cache size #7134

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-4032: memory based rev cache implementation #7040

CBG-4032: memory based rev cache implementation #7040

gregns1 commented Aug 5, 2024 •

edited

Loading

adamcfraser left a comment

adamcfraser Aug 20, 2024

gregns1 Aug 21, 2024

adamcfraser Aug 21, 2024

adamcfraser Aug 20, 2024

adamcfraser Aug 20, 2024

gregns1 Aug 21, 2024

adamcfraser left a comment •

edited

Loading

CBG-4032: memory based rev cache implementation #7040

CBG-4032: memory based rev cache implementation #7040

Conversation

gregns1 commented Aug 5, 2024 • edited Loading

Pre-review checklist

Integration Tests

adamcfraser left a comment

Choose a reason for hiding this comment

adamcfraser Aug 20, 2024

Choose a reason for hiding this comment

gregns1 Aug 21, 2024

Choose a reason for hiding this comment

adamcfraser Aug 21, 2024

Choose a reason for hiding this comment

adamcfraser Aug 20, 2024

Choose a reason for hiding this comment

adamcfraser Aug 20, 2024

Choose a reason for hiding this comment

gregns1 Aug 21, 2024

Choose a reason for hiding this comment

adamcfraser left a comment • edited Loading

Choose a reason for hiding this comment

gregns1 commented Aug 5, 2024 •

edited

Loading

adamcfraser left a comment •

edited

Loading