[3.3.2] CBG-5035: Add revcache value lock for synchronized delta generation #7912
Conversation
…lta generation
There was a problem hiding this comment.
Pull request overview
This PR implements a locking mechanism to synchronize delta generation for the same revision across multiple concurrent clients. The change prevents redundant delta computations when multiple clients simultaneously request the same delta by introducing a per-revision cache value lock.
Key changes:
- Added a
deltaLockmutex torevCacheValueto synchronize delta generation per fromRevision - Refactored
GetDeltato acquire the lock before delta generation and check for cached results - Added comprehensive tests and benchmarks to validate concurrent delta generation behavior
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| db/revision_cache_lru.go | Adds deltaLock mutex to revCacheValue struct and passes it to DocumentRevision |
| db/revision_cache_interface.go | Adds RevCacheValueDeltaLock field to DocumentRevision struct |
| db/crud.go | Refactors GetDelta to use the new lock for synchronized delta generation with double-check pattern |
| db/database_test.go | Adds test and benchmarks for concurrent delta sync cache population scenarios |
| if fromRevision.Delta != nil { | ||
| if fromRevision.Delta.ToRevID == toRevID { | ||
| if initialFromRevision.BodyBytes != nil { | ||
| // Acquire a delta lock to generate delta (ensuring only one toRev unmarshalling/diff for this fromRev and allow racing clients to share the result) |
There was a problem hiding this comment.
Corrected spelling of 'unmarshalling' to 'unmarshaling' to match Go conventions.
| // Acquire a delta lock to generate delta (ensuring only one toRev unmarshalling/diff for this fromRev and allow racing clients to share the result) | |
| // Acquire a delta lock to generate delta (ensuring only one toRev unmarshaling/diff for this fromRev and allow racing clients to share the result) |
| } | ||
|
|
||
| // We didn't unmarshal fromBody earlier (in case we could get by with just the delta), so need do it now | ||
| // We didn't unmarshal fromBody earlier (in case we could get by with just the delta), so need do it now. |
There was a problem hiding this comment.
Missing 'to' in 'need do it now' should be 'need to do it now'.
| } | ||
|
|
||
| // We didn't unmarshal toBody earlier (in case we could get by with just the delta), so need do it now | ||
| // We didn't unmarshal toBody earlier (in case we could get by with just the delta), so need do it now. |
There was a problem hiding this comment.
Missing 'to' in 'need do it now' should be 'need to do it now'.
| //{ | ||
| // name: "5MBDoc", | ||
| // docSize: 5 * 1024 * 1024, | ||
| //}, |
There was a problem hiding this comment.
Remove commented-out test case or add a comment explaining why it's disabled.
|
|
|
I think the 4.x version of GetDelta is returning |
CBG-5035
Unclean (please double-check) cherry-pick of #7903 for 3.3.2
Conflicts mostly around CV handling
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/187/