-
Notifications
You must be signed in to change notification settings - Fork 11.3k
feat: throttle ProgressBar updates to reduce WebSocket flooding #11504
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
base: master
Are you sure you want to change the base?
feat: throttle ProgressBar updates to reduce WebSocket flooding #11504
Conversation
comfy/utils.py
Outdated
| PROGRESS_THROTTLE_SETTINGS = { | ||
| ProgressPriority.CRITICAL: {"min_interval": 0.0, "min_percent": 0.0}, | ||
| ProgressPriority.HIGH: {"min_interval": 0.1, "min_percent": 0.5}, | ||
| ProgressPriority.LOW: {"min_interval": 0.2, "min_percent": 2.0}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the motivation for differntiating between core and custom nodes? There shouldnt't be too much difference. How does it look at feel if everything just follows LOW or something inbetween like 1% to avoid the stack introspection stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the motivation for differntiating between core and custom nodes? There shouldnt't be too much difference. How does it look at feel if everything just follows LOW or something inbetween like 1% to avoid the stack introspection stuff?
I guess my thought behind that is to ensure that live preview should always be allowed though and not throttled since if all gets throttled the same amount, then we might start dropping live preview updates to frontend which is why they are on crititcal. The rest was primarily me thinking that ensuring core component updates having less risk of being dropped. It could probably be set up to be configurable dynamically as well instead of then high/low priority.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The live preview being absolute makes sense to me. I think the code location stuff splitting between LOW/HIGH is not worth it unless you have some hard data on a particular use case? Just giving everyone what you call high is a clear improvement.
The core source code layout is something that evolves so this extra sources list can easily go stale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most common source of the websocket flooding is just the progress updates - @silveroxides could you simplify this PR to throttle only the pbar updates? Other things can then be iterated on that in the future, but this will at least keep peeps from accidentally creating code that freezes ComfyUI 99% of the time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Kosinkadink It has been simplified now.
6b1458e to
2ff192a
Compare
2ff192a to
2c98bca
Compare
rattus128
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implements smart throttling to reduce WebSocket message flooding while
preserving important updates like preview images.
Priority levels:
The priority is auto-detected from the caller's location in the codebase.
Fixes: Frontend congestion from high-frequency progress bar updates
Result from test scenario with a node that is in LOW priority level after fix
At commit 532e285
Total time taken 21s517ms
After changes:
Total time taken 9s367ms
Before_pbar_fix.mp4
After_pbar_fix.mp4