FIFO cache to support eviction based on memory usage

[dmitsh]

Signed-off-by: Dmitry Shmulevich dmitry.shmulevich@sysdig.com

What this PR does:
Change FIFO cache to evict entries based on memory usage.

Which issue(s) this PR fixes:
Fixes #2316

Checklist

• [ x] Tests updated
• [ x] Documentation added
• [ x] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pracucci]

Thanks @dmitsh. Honestly the change set is larger than what I would have expected. I'm not sure I do understand why we need to change how the cache internally store data, instead of just keeping the current cache size (in bytes) and trigger the eviction based on that. I would suggest to see if there's an easier way to implement the bytes-size-based eviction without having to change the data structures.

[dmitsh]

Thanks @dmitsh. Honestly the change set is larger than what I would have expected. I'm not sure I do understand why we need to change how the cache internally store data, instead of just keeping the current cache size (in bytes) and trigger the eviction based on that. I would suggest to see if there's an easier way to implement the bytes-size-based eviction without having to change the data structures.

@pracucci The reason I cannot keep the internal cache storage implementation is because it is tailored for count-based eviction: we allocate the exact number of entries in the array, and play with indices. With the memory usage based eviction we cannot pre-allocate such an array, because we don't know up-front how many entries might fit in the cache (given the fact that the individual cache items might differ in size).

[khaines]

Thank you @dmitsh for making this work both with the item count and memory limits. The only items I have outstanding for myself is that one test question and the deprecated command flag.

[dmitsh]

@khaines @pracucci What do you think about not deprecating fifocache.size?
We can support both for the same price, and folks will have a choice what to use.
The count-based eviction is more intuitive, and the memory-based eviction can be used in k8s deployments, where memory usage is monitored.
I probably should rename it into max-size-items (and deprecate size).
What do you guys think?

[dmitsh]

@khaines @pracucci I went ahead and renamed fifocache.size to fifocache.max-size-items.
It seems to be the right thing to do - to leave both eviction options available.
I also deprecated fifocache.size, so this is now a breaking change again.
Hopefully it could make it to the 1.0 release (cc @gouthamve)

[pracucci]

Good job @dmitsh. I finally had the time to properly look at it. I left few comments, please take a look.

[pstibrany]

I suggest:

• use https://golang.org/pkg/container/list/?
• simplify Put to use byte slices. This cache is only used with byte slices anyway. (Then Put can be removed and we can only keep Store)
• using byte slices will simplify size computation code

[pracucci]

Thanks @dmitsh for your patience on this! I left few minor comments and then LGTM. Peter also left an interesting feedback about using container/list which may make much sense to simplify it.

[pstibrany]

Thank you for your work on this PR. There is one place where NewFifoCache is called, which now needs to check if returned value is ‘nil’:

cortex/pkg/chunk/cache/cache.go

Line 71 in a8003f0

cache := NewFifoCache(cfg.Prefix+"fifocache", cfg.Fifocache)

[dmitsh]

@pstibrany Thank you for catching this. I've updated the PR.

[pracucci]

Thanks for your hard work! I spotted a couple of issues. A part from this, LGTM.

[pracucci]

I left a minor nit, but the current version LGTM.

[pstibrany]

sizeOf is little more tricky that I'd like.

For example, I don't really think string length should be counted twice, because it's only string descriptor that's copied, but not the actual string data.

Also cap(item.value) is not the full story, as subslice of large slice will report smaller cap, but still keep entire large slice referenced.

int(unsafe.Sizeof(&list.Element{})) – why counting just a pointer and not full Element struct?

Personally I would simplify it to len(item.key) + cap(item.value), since it's just a best-effort guess, ignoring maintainance structs.

[dmitsh]

Thanks, I updated sizeOf.

[pstibrany]

Thank you again for your work. I think this is a nice improvement, and good in its current form.

[pracucci]

LGTM, thanks for your hard work! 🎉