Skip to content

Better cache control #259

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

Closed
NicoKiaru opened this issue Jul 18, 2022 · 13 comments
Closed

Better cache control #259

NicoKiaru opened this issue Jul 18, 2022 · 13 comments

Comments

@NicoKiaru
Copy link
Collaborator

NicoKiaru commented Jul 18, 2022

The goal

I'd like to improve Bdv-Playground and make it capable of handling really big data with low RAM.

Why it's not ideal currently

It's doing ok, but it sometimes fails because some cached sources have a SoftRef policy which is not clearing the memory early enough. When the memory is nearly full, the JVM then spends most of its time in GC calls, and this makes the GUI not responsive and me pretty annoyed. Soft references do not seem to be cleared fast enough in this case, and also, part of the problem comes from the absence of order guarantee when removing cached data : the 'cell' which has been accessed the latest is not necessarily the first removed (see extract from https://www.baeldung.com/java-soft-references):

Though, no guarantees are placed upon the time when a soft reference gets cleared or the order in which a set of such 
references to different objects get cleared. 
As a rule, JVM implementations choose between cleaning of either recently-created or recently-used references.

Where soft references are generated in bdv playground ?

I think the main issue in bdv playground comes from the class RAIHelper which has a softref policy. RAIHelper is used by default in bdv-playground for all cached sources which are not coming from a SpimDataset (Resampled sources, Fused sources).

How to solve the issue ?

1. Quick fix

I could probably just put a BoundSoftRef policy with a fixed amount of cells in RAIHelper, but one of the design problem I have is that I make a cache for each 3D RAI (one cache per resolution level and per timepoint). So if I have 50 timepoints and 5 resolution levels, I create 250 different caches. If I put a bound of 10 cells for each cache, I have in fact a max bound of 2500 cells.

2. Better fix

2.a. A cache for each source

It would be better to have at least a single cache for each Source. Like in the AbstractSpimSource implementation, a key of the cache should contain all the information to access a cached cell (timepoint, level, gridlocation). But I'm a bit lost in all the cache API (cache loader, cache, access flags...)

2.b. A global cache for all SourceAndConverter registered in bdv-playground

I was wondering whether it was possible to implement a single cache that would contain all the cached cells from all bdv-playground sources. Basically a key of this cache would identify:

  • the sourceandconverter object
  • timepoint
  • resolution level
  • grid location

This would allow to invalidate all caches at once if necessary and programmatically. Also, a bounded soft ref policy could be applied all at once and act upon all sources.

There is a potential issue memory leak issue, but I think it should be ok: if a SourceAndConverter object is kept in memory because of its remaining cached cells, the order guarantee on BoundedSoftRef should sooner or later manage to get rid of the remaining cells. Also, when a Source is removed from Bdv-Playground, we could make sure to invalidate its cached cells from the global cache.

So : do you think 2.b. is a good option ? If yes, I'll probably need a bit of help to start it, because as I wrote, I'm a bit lost in the API.

Optionally:

  • I'd like to know the size retained by the cache of a SourceAndConverter (probably very difficult)

ping @tischi @tpietzsch

@tischi
Copy link
Collaborator

tischi commented Jul 19, 2022

I cannot comment deeply, because I have not looked into this in detail yet myself.
But, one thing I can share is that in MoBIE we have a function like this:

public void closeSourceAndConverter( SourceAndConverter< ? > sourceAndConverter, boolean closeImgLoader )
	{
		SourceAndConverterServices.getBdvDisplayService().removeFromAllBdvs( sourceAndConverter );
		String sourceName = sourceAndConverter.getSpimSource().getName();

		if ( closeImgLoader )
		{
			final ImgLoader imgLoader = sourceNameToImgLoader.get( sourceName );
			if ( imgLoader instanceof N5ImageLoader )
			{
				( ( N5ImageLoader ) imgLoader ).close();
			}
			else if ( imgLoader instanceof N5OMEZarrImageLoader )
			{
				( ( N5OMEZarrImageLoader ) imgLoader ).close();
			}
		}

		sourceNameToImgLoader.remove( sourceName );
		sourceNameToSourceAndConverter.remove( sourceName );
		SourceAndConverterServices.getSourceAndConverterService().remove( sourceAndConverter );
	}

We found this necessary to clear up the memory.

@NicoKiaru
Copy link
Collaborator Author

Regarding the close calls to imageloader, in theory it could be taken care of in

if (asd.getSequenceDescription().getImgLoader() instanceof Closeable) {
try {
((Closeable) asd.getSequenceDescription().getImgLoader()).close();
} catch (IOException e) {
e.printStackTrace();
}
}

However the imageloaders need to implement closeable, and this does not seem to be the case... I'll make a PR.

@tischi
Copy link
Collaborator

tischi commented Jul 19, 2022

Where does the Closeable interface come from? I did not know about it...
We would need to add this in mobie-io.

@NicoKiaru
Copy link
Collaborator Author

PR done here:

bigdataviewer/bigdataviewer-core#142

(Closeable is java.io.Closeable. But it's not yet included in the Image Loader, that's why I've made the PR)

@tischi
Copy link
Collaborator

tischi commented Jul 19, 2022

Note that several SAC can use the same ImgLoader, e.g. when we construct a new SAC by transforming a SAC. So I think the SACService should only close an ImgLoader if it is sure that there is no SAC left that would need it. Maybe we need a Map< ImgLoader, Set< SAC > > to keep track of this?

@NicoKiaru
Copy link
Collaborator Author

NicoKiaru commented Jul 19, 2022

Note that several SAC can use the same ImgLoader, e.g. when we construct a new SAC by transforming a SAC. So I think the SACService should only close an ImgLoader if it is sure that there is no SAC left that would need it. Maybe we need a Map< ImgLoader, Set< SAC > > to keep track of this?

Yes, it's already taken care of in the lines before (bdv - pg stores which sources are attached to a spimdataset):

if (asd != null) {
if (this.getSourceAndConverterFromSpimdata(asd).size() == 1) {
// Last one! Time to invalidate the cache (if there's one, meaning, if the image loader
// is a ViewerImageLoader)
if (asd.getSequenceDescription().getImgLoader() instanceof ViewerImgLoader) {
ViewerImgLoader imgLoader = (ViewerImgLoader) (asd.getSequenceDescription().getImgLoader());
if (imgLoader.getCacheControl() instanceof VolatileGlobalCellCache) {
((VolatileGlobalCellCache) (imgLoader.getCacheControl())).clearCache();
}
}
if (asd.getSequenceDescription().getImgLoader() instanceof Closeable) {
try {
((Closeable) asd.getSequenceDescription().getImgLoader()).close();
} catch (IOException e) {
e.printStackTrace();
}
}
}
}

@tischi
Copy link
Collaborator

tischi commented Jul 19, 2022

OK, how would I use this in MoBIE? There we open the SACs ourselves, not via bdv-pl...
Do I have to somehow register a SpimData object with a SAC?
How?

@NicoKiaru
Copy link
Collaborator Author

Do I have to somehow register a SpimData object with a SAC?

You can with this function:

public void linkToSpimData( SourceAndConverter sac, AbstractSpimData asd, int idSetup) {
sacToMetadata.getIfPresent( sac ).put( SPIM_DATA_INFO, new SpimDataInfo(asd, idSetup));
}

@NicoKiaru
Copy link
Collaborator Author

There's now a global cache and globalcacheloader objects which are used by resampled sources and by every image loader which contains a cache field. The cache field is replaced thanks to reflection.

All relevant classes are in the package https://github.com/bigdataviewer/bigdataviewer-playground/tree/master/src/main/java/sc/fiji/bdvpg/cache.

Also : the cache is weighted by the memory thanks to this function:

static long getWeight(Object object) {
if (object instanceof Cell) {
Cell cell = ((Cell) object);
Object data = cell.getData();
if (ShortAccess.class.isInstance(data)) {
return 2 * cell.size();
}
else if (ByteAccess.class.isInstance(data)) {
return cell.size();
}
else if (FloatAccess.class.isInstance(data)) {
return 4 * cell.size();
}
else if (IntAccess.class.isInstance(data)) {
return 4 * cell.size();
}
else {
logger.info("Unknown data class of cell object " + data.getClass());
return cell.size();
}
}
else {
logger.info("Unknown class of cached object " + object.getClass());
return 1;
}
}

@tischi
Copy link
Collaborator

tischi commented Oct 14, 2022

@NicoKiaru Does this work now?
What do I need to do to clear all caches related to one SourceAndConverter ?

@NicoKiaru
Copy link
Collaborator Author

What's your imageloader ? I can tell you if it will work or not ?

If, in your image loader there is a field named cache of type VolatileGlobalCellCache, the caching should work pretty nicely.

If your image loader implements Closeable, the close function will be called if all source and converters from the spimdata object are removed from the source and converter service

@NicoKiaru
Copy link
Collaborator Author

https://github.com/mobie/mobie-io/blob/main/src/main/java/org/embl/mobie/io/ome/zarr/loaders/N5OMEZarrImageLoader.java

Looks ok for the cache. But it does not implement Closeable. Since there already is a close function, just add implement Closeable.

This can directly use the GlobalCache I've implemented (see below)


The issue I see with MultiscaleImage is that you use N5Utils.openVolatile:

https://github.com/mobie/mobie-io/blob/3fd0180fa013141dc5d8370dd365794904bfaf27/src/main/java/org/embl/mobie/io/ome/zarr/hackathon/MultiscaleImage.java#L157

Here the cache mechanism is hidden into N5Utils. In its current form it won't be optimal because the cache size is handled at a per source level. Let's say you have 15 Gb of RAM, and a hundred sources of 10 Gb each:

You can use unbounded soft refs everywhere:

  • the problem is that when you hit the RAM limit, Java is bad at removing efficiently unused refs, and it doesn't know which one to remove.

You can use a bounded number of refs:

  • one problem is that you need to limit drastically each source number of cells kept in cache, because you have to suppose a worst case scenario where all sources are opened and accessed at the same time. A quick computation leads you to 15 / 100 = 150 Mb of cache per source max.
  • 150Mb per source for a 10Gb source is not a lot... You can use soft refs on top of strongly guarded refs , but again, you have soft refs which are a pain to remove (cf above). And if you put a hard limit (no soft refs, but weak refs), you will reload many times the same data, if you have a cache of 150 Mb for browsing 10Gb of data
  • also, the limit you can set is in number of cells (=blocks), not directly in terms of memory. So you need to do a small computation to convert the number of blocks to its data size in Mb.

What you would want ideally, is to set a total amount of memory that can be used by the cache of all sources, and when the max is reached, you want to remove the least useful data first, to let some space for new things. And if the limit is global for all sources, you do not need to care whether one source occupies all the RAM, or if many sources are each using a fraction of the total memory.


That's the problem I solved here:

private boolean replaceSpimDataCacheByGlobalCache(AbstractSpimData<?> asd) {
LoaderCache loaderCache = new GlobalLoaderCache(asd);
BasicImgLoader imageLoader = asd.getSequenceDescription().getImgLoader();
VolatileGlobalCellCache cache = new VolatileGlobalCellCache(10, Math.max(1,
Runtime.getRuntime().availableProcessors() - 1));
// Now override the backingCache field of the VolatileGlobalCellCache
try {
Field backingCacheField = VolatileGlobalCellCache.class.getDeclaredField(
"backingCache");
backingCacheField.setAccessible(true);
backingCacheField.set(cache, loaderCache);
// Now overrides the cache in the ImageLoader
if (imageLoader instanceof Hdf5ImageLoader) {
Field cacheField = Hdf5ImageLoader.class.getDeclaredField("cache");
cacheField.setAccessible(true);
cacheField.set(imageLoader, cache);
return true;
}
else if (imageLoader instanceof N5ImageLoader) {
Field cacheField = N5ImageLoader.class.getDeclaredField("cache");
cacheField.setAccessible(true);
cacheField.set(imageLoader, cache);
return true;
}
else {
Field cacheField = imageLoader.getClass().getDeclaredField("cache");
cacheField.setAccessible(true);
cacheField.set(imageLoader, cache);
return true;
}
}
catch (NoSuchFieldException | IllegalAccessException e) {
e.printStackTrace();
return false;
}
}

I'm replacing the cache field (through reflection) of SpimData with a GlobalLoaderCache which caches cells and has a policy based on max memory, taking into account all spimdata objects, and potentially other sources that are using a GlobalLoaderCache.

The global loader cache is here:

https://github.com/bigdataviewer/bigdataviewer-playground/blob/master/src/main/java/sc/fiji/bdvpg/cache/GlobalLoaderCache.java

And it can use a BoundedHashMap or a Caffeine cache backend.

Also importantly, it measures the memory size of each cell which is added in the cache, assuming all objects are instances of Cell, which is always the case so far:

static long getWeight(Object object) {
if (object instanceof Cell) {
Cell cell = ((Cell) object);
Object data = cell.getData();
if (ShortAccess.class.isInstance(data)) {
return 2 * cell.size();
}
else if (ByteAccess.class.isInstance(data)) {
return cell.size();
}
else if (FloatAccess.class.isInstance(data)) {
return 4 * cell.size();
}
else if (IntAccess.class.isInstance(data)) {
return 4 * cell.size();
}
else {
logger.info("Unknown data class of cell object " + data.getClass());
return cell.size();
}
}
else {
logger.info("Unknown class of cached object " + object.getClass());
return 1;
}
}

So if you have a source with little cells (32x32x32) and other sources with big cells (1024x1024x32), the cache will be know which memory size is occupied.

There's one limitation though: it only works with read-only cache.

If there was a way to specify the LoaderCache to use in N5Utils, then the problem could be solved this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants