search

imglib / imglib2-cache

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

fix an annoying bug that when regularly calling invalidateAll() on a … #22

Open StephanPreibisch opened 1 year ago

StephanPreibisch commented 1 year ago

…SoftRefCache, it throws a NullPointerException. Somehow "ref" can be null, check line 213 while get() is still being called

StephanPreibisch commented 1 year ago

let's maybe discuss this more @tpietzsch @axtimwalde ?

tpietzsch commented 1 year ago

Just to understand better: The invalidataAll() javadoc says: "There must be no concurrent get() operations. This may result in cache corruption and/or a deadlock." Did you make sure of this and are still getting this bug?

axtimwalde commented 1 year ago

We realized that but didn't find a way to synchronize it. I.e. we have to clear the cache while a user draws in BDV. BDV renders prior requests asynchronously, and that interferes with invalidateAll. The best way to address this IMO would be to have a delayed call method in BDV that synchronizes with any rendering. I.e. wait for BDV to finish rendering, then do the call, then continue rendering. That would be specific to BDV though, and the next independent application that consumes something using the cache asynchronously will run into the same issue. Curious what you think would be best here.

On March 25, 2023 7:15:00 AM EDT, Tobias Pietzsch @.***> wrote:

External Email: Use Caution

Just to understand better: The invalidataAll() javadoc says: "There must be no concurrent get() operations. This may result in cache corruption and/or a deadlock." Did you make sure of this and are still getting this bug?

-- Reply to this email directly or view it on GitHub: https://github.com/imglib/imglib2-cache/pull/22#issuecomment-1483797763 You are receiving this because you were mentioned.

Message ID: @.***>

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

tpietzsch commented 1 year ago

I think it either needs to be specific to the application, or all cache accesses must be synchronized.

I would make a Lock that BDV acquires/releases before/after rendering, and that the invalidating code would also acquire/release.

Potentially, the Lock could be created by the cache and locking be done automatically in invalidateAll(). But that would be complicated by having multiple caches, caches layered on top of each other, etc. I think it makes more sense to build the locking completely outside.

To not make it super-specific in the BDV API we could add beforeRender(Runnable) and afterRender(Runnable) methods (where one could pass Runnables that acquire/release the lock).

StephanPreibisch commented 1 year ago

Hi, for now I made a local copy of SoftRefLoaderCache called RobustSoftRefLoaderCache (containing the change above) in hot-knife which temporarily solves the issue.

How should we take it from here?

axtimwalde commented 1 year ago

I think it would be best to have an invokeAfterRender(Runnable) method in BDV that is called immediately if no rendering happens or immediately after rendering finishes and before the next rendering starts. That wouldn't require any additional synchronization.

NicoKiaru commented 1 year ago

@axtimwalde but what if several renderers exist ?

tpietzsch commented 1 year ago

@axtimwalde but what if several renderers exist ?

Yes, very good point. That would be a problem. I would go with the beforeRender(Runnable) and afterRender(Runnable) then. The lock should then be ReentrantReadWriteLock so that multiple renderers can take read locks at the same time. (And invalidateAll would take the write lock)

tpietzsch commented 1 year ago

@StephanPreibisch

How should we take it from here?

I'll implement the beforeRender(Runnable) and afterRender(Runnable) in BDV, then build around that in hotknife. Can you point me to where the invalidateAll() happens and something I can run to test it?

StephanPreibisch commented 1 year ago

Here is the code: https://github.com/saalfeldlab/hot-knife/blob/ee328b3eb0e3d357fdcadba220a6223d02d29a9c/src/main/java/org/janelia/saalfeldlab/hotknife/tools/HeightFieldBrushController.java#L93

StephanPreibisch commented 1 year ago

And the parameters for running PaintHeightField.java: --n5Path=/nrs/hess/render/export/hess.n5 --n5FieldPath=/nrs/hess/render/export/hess.n5 --n5Raw=/render/wafer_52_cut_00030_to_00039/slab_013_all_align_t2_ic___20230328_105504 --n5Field=/heightfields/wafer_52_cut_00030_to_00039/slab_013_all_align_t2_ic___20230328_105504/s1/min --n5FieldOutput=/heightfields_fix/wafer_52_cut_00030_to_00039/slab_013_all_align_t2_ic___20230328_105504/min --scale=2,2,1 --offset=0 --multiSem Guess it's best to make a local copy of the heighfield ... or just ask me to test it ...