CBG-3854: Integration of the skipped sequence slice into the cache #6789

gregns1 · 2024-04-23T14:46:31Z

Integrates new skipped sequence slice into the cache
Adds some additional stats as included in the design doc
Adds some test testing both functionality of the slice but also the new stats
Initial profiling locally shows a big improvement in terms of memory usage with a unit test that causes a jump in 80 million sequences. Not perfect given there are more areas to optimize yet but much better

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/2388/

adamcfraser

A few naming nits, but also a question about perf overhead of stats computation per skipped sequence, and whether they should move to changeCache.updateStats.

base/stats.go

db/change_cache.go

adamcfraser · 2024-04-23T21:35:46Z

db/change_cache.go

+	c.db.DbStats.Cache().SkippedSeqLen.Set(int64(len(c.skippedSeqs.list)))
+	c.db.DbStats.Cache().SkippedSeqCap.Set(int64(cap(c.skippedSeqs.list)))
+	if err == nil {
+		c.db.DbStats.Cache().NumCurrentSeqsSkipped.Add(-1)


It feels like c.skippedSeqs should keep track of the current value of NumCurrentSeqsSkipped internally, instead of having the changeCache try to keep track of it. What do you think?

Have added this and the cumulative value to the skipped sequence struct to be tracked there. I have had to make the NumCurrentSeqsSkipped value on skipped sequences struct an atomic value though due to the concurrency around sequences being added/removed from the skipped list and the background task CleanSkippedSequenceQueue running. I was on fence about having it be an atomic value handled inside the skipped sequence slice code, then have that written to the stat in updateStats() or just having the value written direct inside the change cache each time a sequence is added/removed? On one hand it feels tidier and better to handle it in skipped sequence code (like the current state of the PR) but it feels like we're adding an extra atomic operation to update the stat of this value.

Looking at this with slightly fresher eyes after our meetings, I think I have solution to avoid the race between updateStats() and the skipped sequence code writing to them. I will push up the changes now and let me know what you think 😄

adamcfraser · 2024-04-23T21:47:09Z

db/change_cache.go

+	numSequences := newEntry.getNumSequencesInEntry()
+	c.skippedSeqs.PushSkippedSequenceEntry(newEntry)
+	c.db.DbStats.Cache().SkippedSeqLen.Set(int64(len(c.skippedSeqs.list)))
+	c.db.DbStats.Cache().SkippedSeqCap.Set(int64(cap(c.skippedSeqs.list)))


Updating four stats every time we PushSkipped feels like potentially a lot of overhead since they are all atomic writes. This is particularly true for SkippedSeqCap which we expect to change infrequently, but even for the others I don't know if we need this kind of granularity.

One option would be to only update these stats in changeCache.updateStats (which runs once per minute). That would be fine for sgcollect/stats.log - the question is whether we'd expect a customer to want finer granularity than that, and what the performance overhead would be of updating all these in real time.

Completely forgot changeCache.updateStats was a thing. Updated to use this!

…em naming. Test updates to reflect stat chnages too.

…ppedSequenceEntry

base/stats.go

adamcfraser · 2024-04-25T16:10:28Z

db/change_cache.go

@@ -90,6 +89,10 @@ func (c *changeCache) updateStats(ctx context.Context) {
 	c.db.DbStats.Cache().PendingSeqLen.Set(int64(c.internalStats.pendingSeqLen))
 	c.db.DbStats.CBLReplicationPull().MaxPending.SetIfMax(int64(c.internalStats.maxPending))
 	c.db.DbStats.Cache().HighSeqStable.Set(int64(c._getMaxStableCached(ctx)))
+	c.db.DbStats.Cache().SkippedSeqLen.Set(c.skippedSeqs.getSliceLength())


Is there something non-standard going on with the whitespace here? (spaces instead of tab, maybe?)

I don't think so? I have updated to reflect the changes to have a getStats() call for the skipped slice. I think the spacing in Github could be weird, it looks fine in goland to me.

adamcfraser · 2024-04-25T16:23:55Z

db/change_cache.go

+	base.InfofCtx(ctx, base.KeyCache, "Starting CleanSkippedSequenceQueue for database %s", base.MD(c.db.Name))
+
+	maxWait := int64(c.options.CacheSkippedSeqMaxWait.Seconds())
+	if maxWait < 1 {


This feels like the wrong place for this check - we'd want to do it (once) when CacheSkippedSeqMaxWait is set, instead of every time we trigger Clean. Did you have a test scenario that required this check?

Upon reading this again I don't think its needed. If we end up with a customer setting that low of an interval it will just compact pretty much all entries as the maxWait will be 0. I don't think that is too much different to today if the customer was to set that setting that low.

db/change_cache.go

db/skipped_sequence.go

…6789) * CBG-3854: Integeration of the skipped sequence slice into the cache * remove old chnages not needed anymore + gh actions failures * updates based of review. Move stat update to updateStats and alter soem naming. Test updates to reflect stat chnages too. * remove incorrect incrementing of num current sequences inside PushSkippedSequenceEntry * Fix race condition in updateStats() * updates off review * tidy CleanSkippedSequenceQueue

* CBG-3852: Implementation of single sequence entry for skipped sequence slice (#6747) * CBG-3852: implementation of single sequence entry for skipped sequence slice * minor updates to make setLastSeq no-op for single values * updates for review round * remove not needed comment * updates based off review * CBG-3853: Implementation of range sequence entry for skipped sequence slice (#6764) * CBG-3854: Integration of the skipped sequence slice into the cache (#6789) * CBG-3854: Integeration of the skipped sequence slice into the cache * remove old chnages not needed anymore + gh actions failures * updates based of review. Move stat update to updateStats and alter soem naming. Test updates to reflect stat chnages too. * remove incorrect incrementing of num current sequences inside PushSkippedSequenceEntry * Fix race condition in updateStats() * updates off review * tidy CleanSkippedSequenceQueue * stats update * CBG-3855: Sequence range removal support (#6801) * CBG-3855: support for seqeuence range removal * add comment to function + remove repeated code * refactor error handling, simplify stat calculation update * CBG-3850: Optimise releaseUnusedSequenceRange (#6831) * CBG-3850: Optimise releaseUnusedSequenceRange function + support for ranges in pending queue * updates to add _pushRangeToPending and _removeSubsetOfRangeFromSkipped to process duplicates sequences between unused rnage and pending/skipped lists * fix race condition in test * fix for incorrect logic for removing skipped range * updates based off review * updates to fix incorrect code + add extra test coverage of changes * change error name * CBG-3945: Fix for panic in _clip method on skipped sequence slice (#6842) * fix test to use old api

CBG-3854: Integeration of the skipped sequence slice into the cache

a9d5c48

gregns1 self-assigned this Apr 23, 2024

remove old chnages not needed anymore + gh actions failures

d33333e

adamcfraser changed the title ~~CBG-3854: Integeration of the skipped sequence slice into the cache~~ CBG-3854: Integration of the skipped sequence slice into the cache Apr 23, 2024

gregns1 assigned adamcfraser and unassigned gregns1 Apr 23, 2024

gregns1 requested a review from adamcfraser April 23, 2024 16:00

adamcfraser requested changes Apr 23, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Apr 23, 2024

gregns1 added 2 commits April 24, 2024 11:54

updates based of review. Move stat update to updateStats and alter so…

021f14f

…em naming. Test updates to reflect stat chnages too.

remove incorrect incrementing of num current sequences inside PushSki…

cf36b3a

…ppedSequenceEntry

gregns1 assigned adamcfraser and unassigned gregns1 Apr 24, 2024

Fix race condition in updateStats()

a79f17d

gregns1 force-pushed the CBG-3854 branch from 09e03fc to a79f17d Compare April 24, 2024 16:36

adamcfraser requested changes Apr 25, 2024

View reviewed changes

adamcfraser assigned gregns1 and unassigned adamcfraser Apr 25, 2024

gregns1 added 2 commits April 26, 2024 13:22

updates off review

51baa32

tidy CleanSkippedSequenceQueue

bc9a2c4

gregns1 assigned adamcfraser and unassigned gregns1 Apr 26, 2024

adamcfraser approved these changes Apr 26, 2024

View reviewed changes

adamcfraser merged commit 3ae3cf4 into main Apr 26, 2024
30 checks passed

adamcfraser deleted the CBG-3854 branch April 26, 2024 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CBG-3854: Integration of the skipped sequence slice into the cache #6789

CBG-3854: Integration of the skipped sequence slice into the cache #6789

gregns1 commented Apr 23, 2024 •

edited

Loading

adamcfraser left a comment

adamcfraser Apr 23, 2024

gregns1 Apr 24, 2024 •

edited

Loading

gregns1 Apr 24, 2024

adamcfraser Apr 23, 2024

gregns1 Apr 24, 2024

adamcfraser Apr 25, 2024

gregns1 Apr 26, 2024 •

edited

Loading

adamcfraser Apr 25, 2024

gregns1 Apr 26, 2024

CBG-3854: Integration of the skipped sequence slice into the cache #6789

CBG-3854: Integration of the skipped sequence slice into the cache #6789

Conversation

gregns1 commented Apr 23, 2024 • edited Loading

Pre-review checklist

adamcfraser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregns1 Apr 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregns1 Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregns1 commented Apr 23, 2024 •

edited

Loading

gregns1 Apr 24, 2024 •

edited

Loading

gregns1 Apr 26, 2024 •

edited

Loading