CBG-3352 remove invalid entries from revcache #6390
Conversation
db/revision_cache_lru.go
Outdated
| @@ -147,7 +147,10 @@ func (rc *LRURevisionCache) getFromCache(ctx context.Context, docID, revID strin | |||
| return DocumentRevision{}, nil | |||
| } | |||
|
|
|||
| if value.invalid { | |||
| value.lock.RLock() | |||
There was a problem hiding this comment.
This is the check that has performance implications, particularly on systems with a large number of cores, where acquiring a read lock can require scheduling work across cores. We've seen this in our performance testing in the past, which is how we ended up with the design that attempts to keep the value immutable.
As discussed, the invalidation flag breaks this immutability so we're going to need to change something. I don't think switching back to acquiring a read lock on value is the ideal approach, as invalidation is an uncommon use case, and we'd be taking on that cost on every rev cache read.
torcolvin
changed the title
|
Looks good - thanks! |
| value := rc.getValue(docID, revID, false) | ||
| if value != nil { | ||
| value.setInvalidFlag() | ||
| if value == nil { | ||
| return | ||
| } | ||
| rc.removeValue(value) |
There was a problem hiding this comment.
getValue does a non-zero amount of work to update the LRU, even though we're about to remove the entry, as well as aquiring the revcache lock in both get and remove.
Maybe it's better to implement a remove method that only requires key?
You could essentially do:
key := IDAndRev{DocID: docID, RevID: revID}
rc.lock.Lock()
defer rc.lock.Unlock()
if element := rc.cache[key]; element != nil {
rc.lruList.Remove(element)
}
delete(rc.cache, key)
Change
Invalidatefunction from rev cache to remove the value from the cache. Ran-race10 times without issues.[x] https://jenkins.sgwdev.com/job/SyncGateway-Integration/1987/