Memoizer: ask any ImageReader instances to remove all but the current reader#4254
Memoizer: ask any ImageReader instances to remove all but the current reader#4254melissalinkert wants to merge 1 commit intoome:developfrom
Conversation
:) Nope. I'm generally supportive. I remember having considered this, but probably didn't spend the time to think it through well enough as you have.
This is a good and interesting point, but I imagine that a similar edge case could be found even without dropping instances from ImageReader. Since only a breaking change in the storage layout (new field) would trigger an invalidation, even just a change to the logic that fills a field could actually be something should invalidate the cache.
Combined with the previous point, the only additional option I can think of is that we more fully define the semantics of when |
|
Thanks, @joshmoore.
That's a good point. Maybe at minimum, adding a new reader means updating the version number? |
|
No immediate objections to that other than that it might lead to a bit more churn then we've had to date. On the flip side, we can't necessarily control who has added a reader to ImageReader at runtime. I could imagine additionally allowing OMERO to override the version itself (passing a parameter? environment variable?) since that's a more "controlled" environment, but the general case in B-F land is definitely tricky. |
|
As discussed in today's formats meeting, a next step would be to make the behavior here optional (but not default) so we can start testing in OMERO. |
| public void saveReader(IFormatReader reader) { | ||
| // clean up reader list in any instances of ImageReader | ||
| IFormatReader r = reader; | ||
| while (r instanceof ReaderWrapper || r instanceof ImageReader) { |
There was a problem hiding this comment.
Suggest it would be cleaner to have ReaderWrapper and ImageReader both implement an interface 'ReaderContainer' (or use existing interface) for the method:
IFormatReader getReader();
Then test r for being an instance of that. Slightly bigger change so understand why not done :)
Since ImageReader requires the clean method the interface could have something like:
IFormatReader getCleanReader();
Which is implemented in ImageReader as cleanupReaderList(); return getReader(); and in ReaderWrapper to just return getReader().
Not a major point though.
3 participants
Ported from a private draft, for wider discussion (cc @joshmoore). glencoesoftware/omero-ms-image-region#148 provides some additional context for where this idea is coming from.
To minimally demonstrate the difference, run something like
showinf -cache -minmax -expand -separatewithout this change, and note size of memo file (in same directory as data). Run the same thing twice with this change - once to generate the new memo file (note the size again), and once after a minute or so to verify that the new memo file is used and not overwritten. Note-minmax -expand -separatemimics the reader stack used in OMERO.However, this has the potential to impact anything that uses
Memoizer, including but not limited to OMERO. So ideas of what might break, why this change might be bad in general, or other approaches to consider would all be welcome.One potential problem identified is what happens when there is an existing memo file generated by this change, but new reader (or better type detection) is introduced such that the underlying reader that should be used for the file changes.