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

30x speedup when building extra networks UI on startup #8742

Closed

Conversation

space-nuko
Copy link
Contributor

@space-nuko space-nuko commented Mar 19, 2023

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 .hidden CSS 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

@EfourC
Copy link

EfourC commented Mar 20, 2023

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!).

@space-nuko
Copy link
Contributor Author

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

@dayuii
Copy link

dayuii commented Mar 20, 2023

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?

@EfourC
Copy link

EfourC commented Mar 21, 2023

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:

wPT9dPJfnA

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
Rx46xHWrQq

Finished
uFogWJBIx6

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

Benefits are twofold:

1. Completely eradicates the startup cost of building the UI, it's only
ever built when someone wants to use it for the first time
2. Drastically cuts back on the amount of data sent over the network
when loading the webui page
@space-nuko
Copy link
Contributor Author

Made some changes to address this:

  1. 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 [Bug]: TimeoutError: timed out #8418
  2. 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

@EfourC
Copy link

EfourC commented Mar 21, 2023

Nice work! Just tried and works well, so naturally disregard my previous post considering your latest changes, totally mooted;)

@EfourC
Copy link

EfourC commented Mar 21, 2023

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>

@space-nuko
Copy link
Contributor Author

Forgot that *args is a tuple, fixed

@catboxanon
Copy link
Collaborator

catboxanon commented Mar 22, 2023

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.

@space-nuko
Copy link
Contributor Author

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

@scrumpyman
Copy link

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.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 23, 2023

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

@scrumpyman
Copy link

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.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 23, 2023

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

@scrumpyman
Copy link

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.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 24, 2023

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

@missionfloyd
Copy link
Collaborator

missionfloyd commented Mar 24, 2023

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;
}

@scrumpyman
Copy link

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.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 24, 2023

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 gradio-app/gradio#1432 which I doubt will be available for many months

@space-nuko
Copy link
Contributor Author

@scrumpyman Try ceb0fa8, it implements the CSS toggle you wanted after Gradio sends the HTML over once

@scrumpyman
Copy link

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.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 24, 2023

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

@scrumpyman
Copy link

scrumpyman commented Mar 24, 2023

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.

@space-nuko
Copy link
Contributor Author

@scrumpyman Made it configurable like you wanted

@AUTOMATIC1111
Copy link
Owner

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 9ed04e7 makes the problem go away.

@space-nuko
Copy link
Contributor Author

space-nuko commented Mar 25, 2023

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

@evanjs
Copy link

evanjs commented Apr 11, 2023

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);
     }
 }

@space-nuko
Copy link
Contributor Author

Thanks @evanjs could you make that into a code review suggested change? I'll merge it in (no computer at the moment)

Copy link

@evanjs evanjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't seem to toggle the extra networks panel.
Directly toggling the hidden attribute on extra_networks_section seems to resolve the issue.

javascript/extraNetworks.js Outdated Show resolved Hide resolved
Co-authored-by: Evan Stoll <evanjsx@gmail.com>
@evanjs
Copy link

evanjs commented May 1, 2023

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

@MrKuenning
Copy link

Is this PR compatible with the latest build?

@AUTOMATIC1111
Copy link
Owner

The dev branch should be building HTML only when the button is clicked.

@AUTOMATIC1111
Copy link
Owner

as there is now implementation for this feature in the repo, I'm closing 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
9 participants