-
Notifications
You must be signed in to change notification settings - Fork 186
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
Refactor eval cache with atomic.Value #402
Conversation
Current benchmark
|
rateLimitMap[flagID] = rl | ||
} | ||
if !rl.Limit() { | ||
rl, _ := rateLimitMap.LoadOrStore(flagID, ratelimit.New( |
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.
not sure how important this is, but this will always create a new rate limiter, even if one already exists.
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.
ratelimit.New is pretty fast, probably faster than we do a Load()
and checks ok
. All primitive operation code here:
https://github.com/bsm/ratelimit/blob/master/ratelimit.go#L32-L49
ec.idCache = idCache | ||
ec.keyCache = keyCache | ||
ec.tagCache = tagCache | ||
ec.cache.Store(&cacheContainer{ |
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.
iiuc this is called from Start(). Do you expect Start() do be called concurrently? If not, why is the atomic needed 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.
Start() will call it once, however, there's a cron goroutinue also periodically updates the cache when the cache is heavily being read, so atomic is used here. Previously this is locked by RWMutex.
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.
does it update the cache's values or replaces the entire object?
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.
replaces the whole object
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.
cool
fSet, ok := cache.tagCache[t] | ||
if ok { | ||
for ia, va := range results { | ||
f[ia] = va | ||
for fID, f := range fSet { | ||
results[fID] = f |
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.
FYI, this is the change that fixes #398
Description
GetByTags
so that we are just merging into the temp variable instead of changing the protected cacheHow Has This Been Tested?
Types of changes
Checklist: