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

ui is slow when tilt starts with many resources that have a "pending" disable status #5496

Closed
lizzthabet opened this issue Feb 15, 2022 · 7 comments
Assignees
Labels
bug Something isn't working

Comments

@lizzthabet
Copy link
Contributor

lizzthabet commented Feb 15, 2022

Expected Behavior

The UI should only display enabled resources by default on tilt up, and it shouldn't be sluggish to display them.

Current Behavior

After the initial Tiltfile execution, Tilt create UIResources with a "pending" disable status. The uiresource objects are (theoretically) quickly updated with the correct enabled or disabled status (which may be based on tilt args and Tiltfile config info).

The UI will hide disabled resources by default, but it currently doesn't take into account a pending disable status. It'll treat a "pending" status as "enabled" and display the resource.

If you have many resources (> 75 in my testing) defined in a Tiltfile and are running a small subset of them, this may lead to performance issues in the UI while the disable status is updated for each resource. Some users have reported that the "pending" disabled resources show up for about 1 minute, and it takes another minute or two for all the disabled resources to hide. (Yikes!)

Steps to Reproduce

I've seen the same issue often enough in my own testing (see debug-many-resources repo), but I can't reliably reproduce the "pending" disabled sluggish state. (See #5495 for more info on the general performance issue.)

For now, the most straightforward solution is probably for the UI to treat a "pending" disable status as "disabled." We can discuss other UI treatments and/or how disable status gets populated.

@nicks
Copy link
Member

nicks commented Feb 15, 2022

for what it's worth, I also noticed some non-deterministic sluggishness when you change the disabled set, but still trying to understand where it's coming from. Seems like it takes some time for the change to make it to the UI. I don't think it's because the writes to the API server are slow. But I also don't think it's because the UI is slow. Seems to be something in between the two?

@landism
Copy link
Member

landism commented Feb 15, 2022

After doing some debug logging and profiling when bulk disabling 30 resources (through the UI), I'm pretty consistently seeing:
The backend sends websocket updates w/ disabled UIResources within a second or so.
The frontend takes 9-10 seconds to process the flurry of websocket updates that appear (the latest run I tried had 145 ws messages in 9.5 seconds). The JS profiler indicates this is spent pretty much entirely within React's Component.setState, so presumably re-rendering. The React profiler is crashing for me at the moment, so I haven't dug much further on that.

@nicks
Copy link
Member

nicks commented Feb 15, 2022

Hmmmm... ok i think i understand the problem.

I was under the impression that setState did some amount of queueing/backpressure when it receives many updates. From https://reactjs.org/docs/react-component.html#setstate :

setState() does not always immediately update the component. It may batch or defer the update until later.

However, in my testing, it's not really doing any kind of batching at all. Instead, what's happening is:

  • server sends 1 uiresource update
  • client receives 1 uiresource update
  • client rerenders the whole ui
  • and back to the server for one (1) uiresource update

i think we can add some simple batching on the server to make this a lot better. (there's a bigger systemic issue - why do we need to re-render the whole ui on one uiresource update? - but that doesn't lend itself as well to easy fixes)

@landism
Copy link
Member

landism commented Feb 17, 2022

i think we can add some simple batching on the server to make this a lot better

Do you want to tackle this or shall I?

why do we need to re-render the whole ui on one uiresource update

Even worse! When I disable 30 resources, it takes ~10 seconds, generates ~130 calls to Hud.mergeAppUpdates, and ~2/3 of those are just updating a UIButton!

@nicks
Copy link
Member

nicks commented Feb 22, 2022

should we consider this fixed?

@ottaviohartman
Copy link

Yes! The UI is so much faster. Thanks!

@lizzthabet
Copy link
Contributor Author

thanks, @omh1280, @nicks, and @landism! we can close this issue and #5495 based on the changes we pushed out in v0.25.1. if there are other reports of performance issues with many resources, we can reopen and investigate more table view optimizations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants