search

imglib / imglib2-cache

Cache interfaces and java.lang.ref based implementation.
Other
6 stars 4 forks source link

Add mechanism to shutdown cache, in particular internal threads, to free memory #12

Closed hanslovsky closed 5 years ago

hanslovsky commented 5 years ago

As far as I know, there is currently no way to shutdown a cache. In particular that means that the resources of a CachedCellImg or a volatile wrapped cell cannot be garbage collected because the threads that are created inside IoSync or FetcherThreads are not stopped. In particular for IoSync, where the Writer is an inner class, that means that resources stay alive until the JVM is shutdown.

I think it does make sense to add a shutdown/stop/close/... to make sure that resources are freed when a cache is discarded, e.g. a DiskCachedCellImg is not being used anymore. The question is, at what level shuld taht be introduced? Can it be at the root(s) of the Cache hierarchy, i.e. in AbstractCache and CacheLoader?

One caveat is that downstream caches may not know about the shutdown, e.g. a volatile-wrapped CachedCellImg. It would be the responsibility of the caller to ensure that shutdown is called in a sane way.

Related links: https://github.com/saalfeldlab/paintera/issues/309 https://gitter.im/saalfeldlab/general?at=5d5d832e7d3c163641170b8f https://gitter.im/imglib/imglib2?at=5d5d89cb67969726f93df923

tpietzsch commented 5 years ago

FetcherThreads already has a shutdown() method, and I just implemented SharedQueue.shutdown() https://github.com/bigdataviewer/bigdataviewer-vistools/commit/a6987d49e5373a13f4b70bfc918dda5345a70ade. Because SharedQueue/FetcherThreads can/should be shared between images and you have a handle on them from the outside, we should not add anything to CachedCellImg for this.

For IoSync/DiskCachedCellImg, we should definitely add IoSync.shutdown(). And then probably just add a close()/shutdown() to DiskCachedCellImg that shuts down it's IoSync.

hanslovsky commented 5 years ago

Thanks for looking into this! Let me know if there is anything I can contribute.

FetcherThreads already has a shutdown() method, and I just implemented SharedQueue.shutdown() bigdataviewer/bigdataviewer-vistools@a6987d4. Because SharedQueue/FetcherThreads can/should be shared between images and you have a handle on them from the outside, we should not add anything to CachedCellImg for this.

Great, that should be a fairly easy solution.

  1. Do you have a good intuition for what is a reasonable number of priorities in the SharedQueue constructor? A number that is large enough to hold all expected mipmap levels?
  2. And then, should lower resolution mipmap levels be loaded with higher priority (less data in total and something can be on the screen more quickly)?

For IoSync/DiskCachedCellImg, we should definitely add IoSync.shutdown(). And then probably just add a close()/shutdown() to DiskCachedCellImg that shuts down it's IoSync

Currently, the DiskCachedCellImg does not know its IoSync. It only knows that it has a Cache. We could be more specific about the kind of Cache that is being passed and then require it to be a cache that exposes its loader (or always require an IoSync loader). I could also not use the DiksCachedCellImgFactory and create the Cache where I need to have direct ownership of the IoSync.

tpietzsch commented 5 years ago
  1. Do you have a good intuition for what is a reasonable number of priorities in the SharedQueue constructor? A number that is large enough to hold all expected mipmap levels?

Yes.

  1. And then, should lower resolution mipmap levels be loaded with higher priority (less data in total and something can be on the screen more quickly)?

Yes, that's the idea. At least that's how I do it in BDV.

Currently, the DiskCachedCellImg does not know its IoSync. I would at it as a member variable to DiskCachedCellImg.

If you want to work on this, that would be great.

For IoSync/DiskCachedCellImg, we should definitely add IoSync.shutdown(). And then probably just add a close()/shutdown() to DiskCachedCellImg that shuts down it's IoSync.