Skip to content
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

[Performance, main] High CPU usage with Shapes/Points when napari is in the background #6223

Closed
psobolewskiPhD opened this issue Sep 13, 2023 · 8 comments · Fixed by #6425
Closed
Labels
bug Something isn't working priority-high High priority issue severity-medium Impairs users from using napari to do what they need but napari is not crashing or segfaulti
Milestone

Comments

@psobolewskiPhD
Copy link
Member

🐛 Bug

With Shapes and Points layers, there is very high CPU usage when napari is in the background.
Specifically I get >290% CPU and >30% GPU with napari in the background.
(note this does not occur with 0.4.18)
Git Bisect points to: #5802

@andy-sweet has more sleuthing:

  1. In #5802 Shapes overrides _update_draw, and explicitly calls set_highlight(force=True)
  2. set_highlight emits the highlight event
  3. In the vispy shapes layer, the highlight event is connected to on_highlight_changed
  4. on_highlight_changed sets the vispy node's data, which triggers a redraw, so _update_draw is called again

To Reproduce

Steps to reproduce the behavior:
Shapes:

  1. make a Shapes layer using the GUI
  2. watch CPU go to high whether napari is foreground or background
  3. Add some shapes, which will ameliorate the issue
  4. Switch windows, so napari is background, watch high CPU usage

Points:

  1. make a Points layer using the GUI
  2. CPU usage will be fine
  3. Switch windows, so napari is background, watch high CPU usage

Expected behavior

Minimal if any CPU usage in the background.

Environment

napari: 0.5.0a2.dev255+g8b6d68b0
Platform: macOS-13.3.1-arm64-arm-64bit
System: MacOS 13.3.1
Python: 3.10.10 | packaged by conda-forge | (main, Mar 24 2023, 20:12:31) [Clang 14.0.6 ]
Qt: 5.15.6
PyQt5: 5.15.7
NumPy: 1.24.3
SciPy: 1.10.1
Dask: 2023.4.1
VisPy: 0.13.0
magicgui: 0.7.2
superqt: unknown
in-n-out: 0.1.7
app-model: 0.1.4
npe2: 0.7.1.dev24+g31e8b32

OpenGL:
  - GL version:  2.1 Metal - 83.1
  - MAX_TEXTURE_SIZE: 16384

Screens:
  - screen 1: resolution 1680x1050, scale 2.0

Settings path:
  - /Users/piotrsobolewski/Library/Application Support/napari/napari-dev_a1eb8b76ba95fa16ad06e26097b46b8455dfbf0b/settings.yaml
Plugins:
  - napari: 0.5.0a2.dev109+gd91f4ee5 (77 contributions)
  - napari-console: 0.0.8 (0 contributions)
  - napari-ome-zarr: 0.5.2 (2 contributions)
  - napari-svg: 0.1.6 (2 contributions)
  - napari-ui-tracer: 0.1.2 (2 contributions)

Additional context

Discussion here: https://napari.zulipchat.com/#narrow/stream/212875-general/topic/high.20CPU.20use.20with.20Shapes.20layer

@psobolewskiPhD psobolewskiPhD added the bug Something isn't working label Sep 13, 2023
@Czaki
Copy link
Collaborator

Czaki commented Sep 13, 2023

The #6222 may be useful for debug such cases.

@psobolewskiPhD psobolewskiPhD added priority-high High priority issue severity-medium Impairs users from using napari to do what they need but napari is not crashing or segfaulti labels Sep 13, 2023
@aganders3
Copy link
Contributor

aganders3 commented Sep 13, 2023

The force redraws were added recently in #5737 and #5802. There might be a way to achieve the same thing with an event blocker, but when I tried it was not properly updating the highlights. This seems to work for me:

diff --git a/napari/layers/points/points.py b/napari/layers/points/points.py
index db87678ba..91f5d4dc1 100644
--- a/napari/layers/points/points.py
+++ b/napari/layers/points/points.py
@@ -1601,10 +1601,11 @@ class Points(Layer):
     def _update_draw(
         self, scale_factor, corner_pixels_displayed, shape_threshold
     ):
+        force_set_highlight = self.scale_factor != scale_factor
         super()._update_draw(
             scale_factor, corner_pixels_displayed, shape_threshold
         )
-        self._set_highlight(force=True)
+        self._set_highlight(force=force_set_highlight)
 
     def _get_value(self, position) -> Optional[int]:
         """Index of the point at a given 2D position in data coordinates.
diff --git a/napari/layers/shapes/shapes.py b/napari/layers/shapes/shapes.py
index ffff16775..a351d2b5c 100644
--- a/napari/layers/shapes/shapes.py
+++ b/napari/layers/shapes/shapes.py
@@ -2727,10 +2727,11 @@ class Shapes(Layer):
     def _update_draw(
         self, scale_factor, corner_pixels_displayed, shape_threshold
     ):
+        force_set_highlight = self.scale_factor != scale_factor
         super()._update_draw(
             scale_factor, corner_pixels_displayed, shape_threshold
         )
-        self._set_highlight(force=True)
+        self._set_highlight(force=force_set_highlight)
 
     def _get_value(self, position):
         """Value of the data at a position in data coordinates.

@Carreau
Copy link
Contributor

Carreau commented Sep 14, 2023

May I suggest an approach like #6210 ? It already bring a context manager to batch update.

@aganders3
Copy link
Contributor

I could be wrong, but I'm not sure this issue can be solved only by batching as currently the highlight is updated after drawing using data from the (vispy) draw event.

I do think that's a good idea though. I have discussed a bit with @andy-sweet what it would take to remove this feedback event, such as passing camera information (scale, corner pixels) when slicing. This may help avoid similar problems with multiscale images, for example.

@Carreau
Copy link
Contributor

Carreau commented Sep 14, 2023

Could you try f9f3d95 ? It does seem to work for me at least for idle, and might be something to build upon ?

@aganders3
Copy link
Contributor

aganders3 commented Sep 14, 2023

Unfortunately that doesn't work for me, but I'll look closer at how it's working. As-is it doesn't seem to have correct highlighting behavior - basically points highlights don't seem to show up for me (depending on how they are selected). Also the problem of the highlight thickness (solved by #5737) returns. I can fix the behavior if I modify batch_highlight to instead call self._set_highlight(force=True) but then the original issue from this thread (high CPU usage) returns.

@andy-sweet
Copy link
Member

I have discussed a bit with @andy-sweet what it would take to remove this feedback event, such as passing camera information (scale, corner pixels) when slicing. This may help avoid similar problems with multiscale images, for example.

In the long term, it would be great to get rid of that feedback/recursive loop. But any quick patch is good enough for now given the severity of the bug.

In contrast to multiscale images (where we need corner pixels), I'm actually confused about why the feedback loop is needed for points and shapes.

@aganders3
Copy link
Contributor

aganders3 commented Sep 14, 2023

I think it just needs to update the highlight thickness based on scale_factor, which is set in _update_draw.

Another fix would be to treat scale_factor as an additional property of the selection, adding self._scale_factor_stored similar to the way self._selected_data_stored, self._value_stored, and self._drag_box_stored are used to test whether the highlight needs to be updated:

if (
self.selected_data == self._selected_data_stored
and self._value == self._value_stored
and np.array_equal(self._drag_box, self._drag_box_stored)
) and not force:
return

This still needs _update_draw() to be extended to call _set_highlight, but removes the need to force the update and breaks this cycle.

@psobolewskiPhD psobolewskiPhD added this to the 0.5.0 milestone Nov 8, 2023
@jni jni closed this as completed in #6425 Nov 11, 2023
jni pushed a commit that referenced this issue Nov 11, 2023
…CPU background usage) (#6425)

I agree something like #6234 is a
better long-term fix, but we should do something in the meantime because
the high CPU is pretty severe.

# References and relevant issues
Closes #6223

# Description
This breaks a cycle when updating the highlight on Points and Shapes
layers. The problem was `_set_highlight` being called with `force=True`
in `_update_draw`. This caused a cycle as redrawing the highlights would
again call `_update_draw`. This was added to fix the appearance of the
highlights during interaction - see #6223 for more discussion. I'm
hoping @brisvag can confirm this PR maintains the correct behavior.

This PR tracks `scale_factor` the same way `selected_data` and other
properties are stored in order to conditionally update the highlight
instead of forcing it.
Czaki pushed a commit that referenced this issue Nov 13, 2023
…CPU background usage) (#6425)

I agree something like #6234 is a
better long-term fix, but we should do something in the meantime because
the high CPU is pretty severe.

# References and relevant issues
Closes #6223

# Description
This breaks a cycle when updating the highlight on Points and Shapes
layers. The problem was `_set_highlight` being called with `force=True`
in `_update_draw`. This caused a cycle as redrawing the highlights would
again call `_update_draw`. This was added to fix the appearance of the
highlights during interaction - see #6223 for more discussion. I'm
hoping @brisvag can confirm this PR maintains the correct behavior.

This PR tracks `scale_factor` the same way `selected_data` and other
properties are stored in order to conditionally update the highlight
instead of forcing it.
Czaki pushed a commit that referenced this issue Nov 13, 2023
…CPU background usage) (#6425)

I agree something like #6234 is a
better long-term fix, but we should do something in the meantime because
the high CPU is pretty severe.

# References and relevant issues
Closes #6223

# Description
This breaks a cycle when updating the highlight on Points and Shapes
layers. The problem was `_set_highlight` being called with `force=True`
in `_update_draw`. This caused a cycle as redrawing the highlights would
again call `_update_draw`. This was added to fix the appearance of the
highlights during interaction - see #6223 for more discussion. I'm
hoping @brisvag can confirm this PR maintains the correct behavior.

This PR tracks `scale_factor` the same way `selected_data` and other
properties are stored in order to conditionally update the highlight
instead of forcing it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority-high High priority issue severity-medium Impairs users from using napari to do what they need but napari is not crashing or segfaulti
Projects
None yet
5 participants