improvement: Use LRU instead of maps on defaultStore

[ajnavarro]

Description

Changing actual cache implementation to use an LRU cache improves resource usage and avoids exhaustion.

[tbruyelle]

Can you explain why maps are exhausting plz ?

EDIT: forget that, I didn't notice it was for caching.

[moul]

@ajnavarro, thank you, can you provide some metrics, i.e., benchmark before/after?

Warn: new dependency, to be reviewed.

cc @peter7891 good example of candidate for the performance management framework.

[tbruyelle]

How did you evaluate those values ?

[ajnavarro]

Executing a stress test using supernova. Ideally, we should define a total max memory available for store cache.

[ajnavarro]

@ajnavarro, thank you, can you provide some metrics, i.e., benchmark before/after?

Warn: new dependency, to be reviewed.

cc @peter7891 good example of candidate for the performance management framework.

I can create some benchmarks measuring the total memory used.

[ajnavarro]

@moul @tbruyelle I think I found a bug. After limiting the number of objects allowed to be on memory, some tests started to fail saying that a package is missing from the store (it should be there if it was on the cache before). That might be considered a bug. WDYT?

[tbruyelle]

@moul @tbruyelle I think I found a bug. After limiting the number of objects allowed to be on memory, some tests started to fail saying that a package is missing from the store (it should be there if it was on the cache before). That might be considered a bug. WDYT?

Sure, what is the error exactly ?

[ajnavarro]

I found the problem. The store interface has a SetCachePackage method that is only intended to temporally store the package. Before that change, it was stored in memory with no limits, adding the possibility of an OOM error. With this change, we are evicting less used objects, so after 100 packages are stored in memory we remove the less used ones, causing tests to fail because the temporal package was evicted.

IMHO a better solution would be to store these temporal packages on a persistent storage that can be deleted eventually.

[tbruyelle]

OK so to be sure I understand well, some packages (I assume the ones added via SetCachePackage()) aren't retrievable from the store via loadObjectSafe() right ?

[ajnavarro]

Hmm, loadObjectSafe() was never able to obtain packages stored using the SetCachePackage() method.

The problem with the actual change is that SetCachePackage is using an in-memory cache that was not expecting to be evicted in any case, but with that change adding an LRU cache we are discarding less used packages from memory.

GetPackage is looking for packages from memory and from other two sources if they are present: baseStore and pkgGetter. With this change, we are removing some packages from memory, so it is not able to find them.

[tbruyelle]

OK that's what I understood, the packages that are manually put in cache via SetCachePackage, are not retrievable from the store or from the pkgGetter. Maybe the solution is to understand why they aren't retrievable from these sources, and then make sure they will be (via addionnal code in SetCachePackage maybe?).

[ajnavarro]

I asked @zivkovicmilos to make this PR a draft because I'll need more time to think about throwaway packages.

[ajnavarro]

Closing in favor of #812 to try to define the best solution.