-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix rampant memory leak in providers records storage #1315
Conversation
pi.Value = np.val | ||
arr := pm.providers[np.k] | ||
pm.providers[np.k] = append(arr, pi) | ||
provs, ok := pm.providers[np.k] |
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.
pm.run
is the only func that access this map, right? might want to document that in the struct
in the future, i do want to move back to using arrays here, maps use a lot of memory, and iterating maps into an array for every request is moderately expensive. But doing so would require a limit on the number of providers stored for any given key to avoid the problem i'm fixing here again (checking for existence in a large array is unpleasant, but doable) Maybe I should just implement that now? |
delete(provs, p) | ||
} else { | ||
parr = append(parr, p) | ||
} |
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.
this is making a huge array every time, isn't it? nevermind
@whyrusleeping what i do for this sort of thing is keep both. something like: var ProviderLifetime = time.Hour * 24
// record key : providers
var providers = map[u.Key]*ProviderSet
type ProviderSet struct {
records []Record // to be copied right into a message
set map[u.Key]time.Time // set for fast checking.
}
func (ps *ProviderSet) add(r Record) {
if _, found := ps.set[r.key]; !found {
ps.records = append(ps.records, r)
}
ps.set[r.key] = time.Now()
}
func (ps *ProviderSet) expire() {
// cleanup both ps.set and ps.records for entries whose time is > ProviderLifetime
} the memory usage is not prohibitive and yields higher req/s |
filtered = append(filtered, p) | ||
for _, provs := range pm.providers { | ||
for p, t := range provs { | ||
if time.Now().Sub(t) > time.Hour*24 { |
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.
constant:
var ProviderLifetime = time.Hour*24
I would argue that the memory usage on that is prohibitive, we are going to have to store a lot of providers (or set limits, which opens pandoras box). a 2x bump in memory usage is gonna hurt. |
but i guess we can cross that bridge when we get there. I'll go ahead and implement the map/slice combo. |
d796377
to
2030d22
Compare
address comments from CR use map and array combo for better perf
2030d22
to
abb85fc
Compare
fix rampant memory leak in providers records storage
This was broken, its better now. provider records still arent signed, but now our gateways wont inadvertently run themselves out of memory.