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 needs to be improved #143

Closed
liamhuber opened this issue Dec 1, 2022 · 5 comments
Closed

Performance needs to be improved #143

liamhuber opened this issue Dec 1, 2022 · 5 comments
Assignees

Comments

@liamhuber
Copy link
Member

Selecting nodes with lots of ports takes a long time, I believe the holdup is generating the node controller interface. This needs to be made more performant.

@liamhuber liamhuber self-assigned this Dec 1, 2022
@liamhuber liamhuber changed the title Performance sucks for nodes with lots of ports Performance needs to be improved Dec 14, 2022
@liamhuber
Copy link
Member Author

liamhuber commented Dec 14, 2022

In ironflow.gui.workflows.boxes.node_interface.representation.NodePresenter._draw, we now recreate the representation toggles and output widgets at every draw call. This is because we have a single flag representation_updated that's now doing double-duty to communicate both/either a representation value has been updated and/or the space of representations has changed (i.e. we've un/batched the node).

First, I'd like to switch over to using ryvencore.Base.Event (in many places, not just here), and second this should be split into two events: the value update (which only needs to update the field values), and the space update (which needs to redraw basically the whole presenter widget).

EDIT: Ok, this full redraw now only happens when the length of the representations changes. Would still be better to do it with an event though...

@liamhuber
Copy link
Member Author

This is largely resolved in #152, which plugged a memory leak with more and more widgets getting added in the background as you worked with the graph.

There are some less serious outstanding issues:

  • Opening the node controller for large (lots of IO) nodes is still sluggish -- probably the creator calls can be optimized
  • Moving nodes around on the graph is sluggish, especially when there's lots there -- probably being more clever with ryven Events and ipycanvas hold_canvas will resolve this
  • Despite Performance improvements #152, reinstantiating the GUI does still pile up the number of secret widgets -- probably there is some reference to these widgets that I'm not seeing and killing, and once it's found and resolved reinstantiating the gui will work as well as re-opening the node presenter/controller.

@liamhuber
Copy link
Member Author

Note: Placing a node with default values seems to trigger the update function for each input value, and not once for all at once. The update function is pretty lightweight so I would be surprised if this makes a huge difference, but it's an issue that correlates well with the larger nodes taking longer to place.

@liamhuber
Copy link
Member Author

I suspect the biggest issue right now is that calls to draw ports check the port status (either dtype and or otype). Neither is prohibitively expensive, but checking the ontological type can really take many ms. This can probably be fixed with some sort of event-system so that the port validity is only updated on connections/updates (i.e. a model event), instead of on a draw call (a UI event).

Depending what further improvements we can get in pyiron_ontology, a worst-case fallback position is also to set ontological typing as an activated ability with a special button rather than something that happens automatically. I'd prefer to fully exhaust automated possibilities first though.

@liamhuber
Copy link
Member Author

liamhuber commented Apr 5, 2023

So there were two big issues:

  1. The port dtype and otype checks were integrated with the draw logic, so this was being re-computed every draw cycle for the graph canvas! This made moving nodes around on the graph impossible. This was resolved in Don't update nodes when drawing the controller #180
  2. The logging turned out to be really expensive. The main reason for this was the use of ipywidgets.Output and its append_stdout method, which just added more and more elements to the outputs field and gave horrible scaling as the number of log lines increased. Using ipywidgets.HTML makes things livable again -- O(100ms) to add a new node -- and gives ok scaling. The other issue is just that IO is a slowish thing to do. We can get around this by setting enable_ryven_log to False by default -- O(10ms) to add a new node -- without removing the functionality entirely. This is done in More performant logging #179.

The ontological update status is still pretty slow, and scales poorly with the size of number of ontologically typed ports in the graph. However, the lags are quite livable so I started a separate issue for that (#182) and will close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant