stable-diffusion-webui
stable-diffusion-webui copied to clipboard
AUTOMATIC1111
30x speedup when building extra networks UI on startup
Describe what this pull request is trying to achieve.
The extra networks page builder was stupidly unoptimized, I fixed how it concats the final page. Got 33it/s before, now I'm up to 1099it/s
Also adds a progress bar for building the UI so it's clear it takes up that much time
EDIT: Now I get "infinite" speedup, the extra networks UI is no longer built on startup so there's no startup impact anymore, it will only be loaded when someone clicks the button for the first time. Plus this fixes another issue, the extra networks UI being so bloated it causes network timeouts from how much data it adds to the initial app bundle (#8418), and it should greatly improve page load/refresh time if thousands of extra networks are scanned
Closes #8037, closes #8781
Additional notes and description of your changes
- Changed from concatenating with
+=in a loop to using"".join(html) - Extra networks pages are now only built when someone clicks the button in the UI
- Created a hook into the extra networks button that toggles the
.hiddenCSS instead of sending the entire HTML to the Gradio backend once the extra networks pages HTML has been rendered for the first time, improves performance by bypassing the network call
Environment this was tested in
List the environment you have developed / tested this on. As per the contributing page, changes should be able to work on Windows out of the box.
- OS: Windows
- Browser: Chrome
- Graphics card: NVIDIA RTX 3090
Screenshots or videos of your changes N/A
This is nice, though it's a bit too busy on the printouts during startup (I would personally still use it with no changes rather than not have it, however!).
Yeah reason I added those was because I was initially confused why my webui was hanging for like 3 minutes on startup, that at least gives some visual insight on the time spent
This change was very useful to me, reducing the startup time from 130 seconds to 27 seconds. Can i request additional functionality, sort as arranging extra networks by date?
I was thinking about this a little more while learning about tqdm and realized that since the startup logs will be copy/pasted into countless threads (not to mention everyone will see them every startup, want to or not), it's probably a good idea to tighten up the progress printouts for this.
Below are a couple options I thought of (I prefer option 2). What do you think?
(Edit: Made it a collapsible section to make post cleaner)
Options...
Option 1.
At a minimum, move the per-bar titles on separate lines into the progress line itself with the desc=self.title parameter in the tqdm call:
Option 2.
Make the progress bars only visible while actively going through each category by using the leave=False param in the tqdm call (along with 'desc'), and display the total elapsed time at the end of each tab:
In progress
Finished
So change 2 lines in ui_extra_networks.py, create_html:
def create_html(self, tabname):
...
# Remove the 'Loading extra networks page' printout on this line, and modify tqdm() call below
for item in tqdm.tqdm(self.list_items(), desc=self.title, total=self.item_count(), leave=False):
items_html.append(self.create_html_for_item(item, tabname))
...
And add a Timer and printout in ui_extra_networks.py, create_ui:
def create_ui(container, button, tabname):
...
timer = Timer() # <-- New
with gr.Tabs(elem_id=tabname+"_extra_tabs") as tabs:
print(f"Building extra networks UI for {tabname} tab...")
for page in ui.stored_extra_pages:
with gr.Tab(page.title):
page_elem = gr.HTML(page.create_html(ui.tabname))
ui.pages.append(page_elem)
print(f"Finished in {timer.elapsed():0.2f}s.") # <-- New
...
And of course from modules.timer import Timer
Made some changes to address this:
- Now the extra networks UI is not generated at startup at all, it's only ever loaded when someone clicks the button for the first time. This cuts the startup cost to zero and fixes network bloat/timeouts that cause crashes like #8418
- Removed the tqdm bars unless greater than 1,000 networks in a category are present, even so the bars will only appear in the logs if one opens the extra networks UI, so this change shouldn't touch the startup logs anymore
Nice work! Just tried and works well, so naturally disregard my previous post considering your latest changes, totally mooted;)
Hit an error when I tried to toggle close the extra networks display:
Error printout
File "N:\stable-diffusion-webui\venv\lib\site-packages\gradio\routes.py", line 337, in run_predict
output = await app.get_blocks().process_api(
File "N:\stable-diffusion-webui\venv\lib\site-packages\gradio\blocks.py", line 1015, in process_api
result = await self.call_function(
File "N:\stable-diffusion-webui\venv\lib\site-packages\gradio\blocks.py", line 833, in call_function
prediction = await anyio.to_thread.run_sync(
File "N:\stable-diffusion-webui\venv\lib\site-packages\anyio\to_thread.py", line 31, in run_sync
return await get_asynclib().run_sync_in_worker_thread(
File "N:\stable-diffusion-webui\venv\lib\site-packages\anyio\_backends\_asyncio.py", line 937, in run_sync_in_worker_thread
return await future
File "N:\stable-diffusion-webui\venv\lib\site-packages\anyio\_backends\_asyncio.py", line 867, in run
result = context.run(func, *args)
File "N:\stable-diffusion-webui\modules\ui_extra_networks.py", line 259, in toggle_visibility
return [is_visible, has_loaded, gr.update(visible=is_visible)] + pages
TypeError: can only concatenate list (not "tuple") to list```
</details>
Might be something to address in a future PR instead, but I would suggest giving the user an option whether they want to scan the models for metadata or not. I personally don't find the Show metadata button that useful in the extra networks UI and instead am using kohya's extension to view that data. For users with a lot of files it would cut down a lot on disk reads and output HTML size and give even more speedup.
WebUI should probably store that kind of thing in a proper database like SQL to not have to on scan every startup. Maybe keeping track of newly added files
Startup is much faster, but now opening AND closing the extra network UI is much slower, like 4 seconds every time. Building the UI only when the button is clicked the first time is fine, but then it should be saved after that.
The bottleneck in that case isn't the extra networks page being created, it's the fact that it's extremely inefficient to load/unload 60MB of raw HTML from the UI every time the button is pressed (I measured it, on my system). That has more to do with the available options in Gradio to create such a UI being limited, and the current implementation of the UI being horribly inefficient and bloated. Personally I don't find it very useful since I have hundreds of LoRAs and they take up a gigantic amount of screen space, so much so it's impractical to use in any real capacity
At least, it would be better to implement a rudimentary pagination for the UI so it fits on one screen and doesn't take forever to load. I think that should be a separate change though, it's more important to not waste a ton of time every startup waiting for the HTML to load, which is what this PR fixes
I don't know anything about Gradio, but I'm thinking that after the first loading of the extra networks ui, all subsequent presses of the icon should only hide or show that section of the html and not unload it.
What I mean by "unload" is unload it from the browser's DOM, the actual HTML of the pages still remains on the backend
When the extra networks pages are shown/hidden, the Gradio HTML component on the backend has send the updated text of the HTML component to the frontend (UI), which could be a huge amount of data, then the browser has to add/remove all the components contained within the pages HTML and update its state. Both those actions cause performance issues if theres a lot of HTML to render all at once
Yeah I mean, you have subsequent presses hide/show the ui through css, which I think is how it's done normally. I don't see why it couldn't be done to fallback to normal method once the first loading is done.
Yeah the way it was done before was pre-rendering the HTML, then only toggling the visibility of the cards in CSS. This is faster to toggle on and off like you say, but it incurs the startup cost and network bloat, which is a worse issue IMO
Now the HTML is sent through Gradio as a state change which causes the toggle slowdown
I think it's because with gradio the event handler always has to send the full state of the component, it can't get away with ignoring the component after first press, even if nothing has changed on the backend, unless you were to do some hackish thing like delete the onclick handler in JS
Again I think this can be solved fairly easily once this PR is merged by just limiting the max amount of cards on-screen and paginating
they take up a gigantic amount of screen space
My solution to that particular problem was to make it scroll.
#txt2img_extra_tabs > .tabitem, #img2img_extra_tabs > .tabitem {
max-height: 24em;
overflow: auto;
}
I feel like pagination would mess up the search functionality/speed, which is pretty much the most useful part of the extra networks ui. I really can't believe you can't just keep the ui in DOM after loading it. It's just moving the ui building from startup to before the ui toggle (if it's not already built), and the ui toggle is left the same, no? If that really isn't possible because of Gradio being stupid, then that change (moving ui building away from startup) should be separate from the previous improvement to the speed increase of the ui building.
When I tested without that change (sending the built UI when button is pressed) there is still a huge page load/refresh penalty of 10-15s because the entire prebuilt HTML will be sent over the network every time the app starts
The CSS way of hiding the page after first load can probably be done with enough JS hacking, I just wish it weren't necessary. Sadly I don't think it's possible to do entirely through gradio since it expects the full state of the UI to be sent every time you interact with one of the elements, even if the HTML is fully hidden the entire body has to be sent with every request, its just the nature of their design
At some point the UI has to change to send only a partial amount of the HTML for a single page to be performant, whether through pagination or some sort of reactive interface that can dynamically append elements, the former can be accomplished right now with support for a search box as the Image Browser extension has shown and the latter is likely going to be dependent on https://github.com/gradio-app/gradio/issues/1432 which I doubt will be available for many months
@scrumpyman Try ceb0fa8, it implements the CSS toggle you wanted after Gradio sends the HTML over once
That works, but thinking further, I think moving the ui building away from startup should still be separate. Maybe as an option? I don't think it's a global upgrade for everyone, since some may prefer to have the loading be done on startup instead of when they want to use the ui. Also, is the ui built every time the web page is refreshed without this pr? Using this pr it isn't kept between refresh, which could be a slight annoyance too.
I think moving the ui building away from startup should still be separate. Maybe as an option? I don't think it's a global upgrade for everyone, since some may prefer to have the loading be done on startup instead of when they want to use the ui.
I still don't see why it's necessary for it to be an option, but given the current state of the UI I guess it won't hurt. If the card list used pagination or infinite scrolling I honestly don't think such an option would be needed
Also, is the ui built every time the web page is refreshed without this pr? Using this pr it isn't kept between refresh, which could be a slight annoyance too.
The page HTML is built on the backend only once. The issue is, when the page is first loaded the extra networks HTML has to be sent to the frontend somehow. That must happen regardless of whether the pages are built on startup or when the button is first clicked. The cost of loading the HTML comes from the fact that it can balloon to huge sizes and it's all rendered at once. The way it's implemented right now is really inefficient. Limiting it by page bounds the maximum time it can take for the UI to load
And if you restart webui a lot, for example if you're developing, and never intend to use the extra networks UI, then the extra time taken to render it on page refresh is wasted
It's good to have it as an option if a considerable amount of users would find it preferable one way or the other, which I think would be the case. A lot of people would prefer to add a few or not so few seconds to startup than to have the ui freeze when actively using it, and a lot of others never want to use the extra networks ui at all.
Of course, if a proper database and pagination system is made, that would be best, but that seems like it would take a while so in the meantime having the optimization merged and maybe an option to offload the ui building would be useful.
My preferred solution to this is to use a network request to retrieve metadata. This way the initial page size is small and hopefully no settings are needed. See if 9ed04e759d8b4a84db1f0e37abee59178fe1f586 makes the problem go away.
Here are my results, using current master and a version that doesn't call page.create_html on startup
master:
Startup time: 65.9s (import torch: 2.2s, import gradio: 1.4s, import ldm: 0.5s, other imports: 1.4s, list extensions: 4.4s, setup codeformer: 0.1s, load scripts: 16.0s, load SD checkpoint: 4.6s, create ui: 30.6s, gradio launch: 4.7s).
changed:
Startup time: 51.5s (import torch: 2.3s, import gradio: 1.3s, import ldm: 0.4s, other imports: 1.3s, list extensions: 4.4s, setup codeformer: 0.1s, load scripts: 16.1s, load SD checkpoint: 4.4s, create ui: 16.9s, gradio launch: 4.2s).
So it still takes 13.7 seconds longer. I'd say that's worth the changes for, but up to you
A bit too lazy to check what is actually going wrong/if this is a valid fix, but I was initially unable to toggle the extra networks section back off, though opening it the first time seemed to work fine.
diff --git a/javascript/extraNetworks.js b/javascript/extraNetworks.js
index eaefe2d1..96931c3d 100644
--- a/javascript/extraNetworks.js
+++ b/javascript/extraNetworks.js
@@ -198,7 +198,7 @@ function extraNetworksHookPageToggleIfBuilt(tab_name) {
// Add our own event to toggle extra networks section with CSS instead of Gradio
new_button.addEventListener("click", function() {
- extra_networks_section.classList.toggle("hidden");
+ extra_networks_section.hidden = !extra_networks_section.hidden;
}, false);
}
}
Thanks @evanjs could you make that into a code review suggested change? I'll merge it in (no computer at the moment)
Not sure why I didn't notice it before, but I am hitting NotImplementedErrors, primarily from https://github.com/AUTOMATIC1111/stable-diffusion-webui/pull/8742/files#diff-117b4f8ba2ca615dbf396c924db1196dc653e2fdcbd9ad50d367b38e061d897eR149, IIUC
Do we expect item_count to remain unimplemented?
If so, I'd like to verify what triggers the issue and how it can be avoided 🤔
Either way, changes from dev have landed, so some updates are required for this PR regardless to address new merge conflicts
