Fix invalidate of extent cache in Layers #7972
Conversation
| for layer in ll.selection: | ||
| layer.refresh(data_displayed=False) |
There was a problem hiding this comment.
would it be possible/good to only fire this for layers where something actually changed?
There was a problem hiding this comment.
hm. I think it may require more refactor. And current change already reduce number of refresh call.
| self.events.extent() | ||
|
|
||
| def _clear_extent_augmented(self) -> None: | ||
| """Clear extent_augmented cache and emit extent_augmented event.""" | ||
| if '_extent_augmented' in self.__dict__: |
There was a problem hiding this comment.
Why this change? The separation is useful for cases like Points where changing the size does not actually modify the extent, only the extent augmented.
There was a problem hiding this comment.
To not call 2 functions in each update place.
I do not spot the case of Points. Will update it later.
But I just remove function, and no test crash so it is not called in any place except refresh, that it is called next to just _clear_extent
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7972 +/- ##
==========================================
+ Coverage 92.91% 92.95% +0.04%
==========================================
Files 647 647
Lines 60875 60917 +42
==========================================
+ Hits 56560 56625 +65
+ Misses 4315 4292 -23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The more I look around the more I'm confused... How did you notice this problem, since Also, I would think |
| ] | ||
| elif isinstance(loaded_attr_value, np.ndarray): | ||
| if loaded_attr_value.ndim == 1: | ||
| with layer._block_refresh(): |
There was a problem hiding this comment.
@brisvag You may see here where I want to use block of refresh until all properties are updated as calling refresh triggers multiple heavy computation.
I intend to avoid refresh, so force is exactly this that I do not want to use.
There was a problem hiding this comment.
Sorry, I looked at this PR in 3 different moments and lost track 😅
But you can do force and pass only extent as true, and this keeps everything tidy inside the refresh method. I would remove the whole refresh_blocked machinery, and then do
layer.refresh(extent=True, thumbnail=False, data_displayed=False, highlight=False)There was a problem hiding this comment.
We already do that ina number of places, like when swapping shapes mode to refresh highlights only
There was a problem hiding this comment.
No. We cannot do this. In general scenario, the change of scale or units impact rendering and may require reload of data. However, in this specific case, such reload are just obsolete CPU cycles that only slowdown code execution as there are other attributes that may be modified too. So we want to have only one data reload, not more.
There was a problem hiding this comment.
It's literally identical though:
layer.refresh(extent=True, thumbnail=False, data_displayed=False, highlight=False)Is exactly equivalent to (and ONLY to):
self._clear_extent()
self._clear_extent_augmented()and we'd be using public api and a single source for refreshing isntead of having cache invalidation scattered all over the place. Also, it would allows us to remove this refresh_blocked part.
Unless you refer to if extent: ... as "obsolete CPU cycles", but there's no way that's even close to being relevant here O.o
There was a problem hiding this comment.
No it is not equivalent.
Please see this line:
napari/src/napari/layers/base/base.py
Lines 1520 to 1522 in e44bc01
There was a problem hiding this comment.
We need to always invalidate extent, and sometimes refresh view of data.
There was a problem hiding this comment.
I thinkwe lost track of my point here xD Waht I meant was to remove the _refresh_blocked machinery (both the guard and the context manager, and instead only use the line I suggested. I f refresh blocked is removed, that becomes equivalent.
The reason I'm insisiting on this is that the whole reason why #7015 was made was to centralize the spot where refresh happens so that it's easier to reason about, test, and maintain. Having cache invalidation scattered around makes bugs harder to hunt down. Having extra machinery to block refreshing, also. All the parameters in refresh() were added specifically to solve this problem of "I want to refresh but only the important bits".
If you still think my solution is not equivalent, maybe let's hop onto a call so you can explain why, cause I don't see the problem at the moment :P
There was a problem hiding this comment.
If you still think my solution is not equivalent, maybe let's hop onto a call so you can explain why, cause I don't see the problem at the moment :P
Simple example.
You have multiscale data, where only visible part of data is loaded to GPU. When you change scale, you need to select chunks sended to GPU.
Hoverer when you want to update multiple parameters (ex both scale and transform) you want to wait with calculation and fetch of new chunk until both parameters are set.
In the ideal world, such updates such changes should not trigger any data fetch until painting next frame, but it requires refactor, that is already planned for napari-lite.
There was a problem hiding this comment.
Ok we had a call, and I understand the issue now. To summarize my understanding, the issue is that setting scale, translate etc in a layer uses property setters which will fire refresh() (all args as True). This results in many recalculations of not only the extents, but also the slicing and lots of potentially expensive calculation.
Ultimately, the right way to solve this is to have some nice system for delaying/queuing/deduplicating events, but most importantly to remove slicing state from layers.
Since those are both big undertakings, this PR is the best for now :)
References and relevant issues
Description
In #7015 the change from cleaning extent to refresh it miss that
_block_refreshprevents from invalidating cache when a callback connected to event expects it.It also updates pasting spacing information to reduce layer refresh.