stable-diffusion-webui icon indicating copy to clipboard operation
stable-diffusion-webui copied to clipboard

reduce script extensions loading time using threadpoolexecutor

Open wkpark opened this issue 1 year ago • 3 comments

Description

  • [x] reduce script extensions loading time using ThreadPoolExecutor
    • at loading extension modules step, it could be parallelized.
  • [x] add script_loading_max_thread "System" option. (set max thread =1 for normal loading)

Notes: Due to the fact that sys.path is required with Lock(), the effect of max_thread>2 is not quite large.

Results:

test condition:

  • with all builtin extensions
  • +controlnet + dynamic_prompt + supermerge + loractl + ...

without ThreadPoolExecutor() : script_loading_max_thread=1 ~16.5 sec load scripts time.

image

with ThreadPoolExecutor(): script_loading_max_thread=4 ~10 sec load scripts time. image

Checklist:

wkpark avatar Oct 12 '24 09:10 wkpark

for most script it's fine but similer to your other #16548, this will call scripts be loaded in undetermined order

some scripts are designed in a way that they are loaded in a specific order otherwise it breaks

sometimes the order within an extension itself (IMO this means that the extension has multiple coded poorly) bout sometimes require other extensions to be loaded first

you mean, some extensions have execution code while loading time and that could cause issue?

yes. you are right, some extensions have execution code like as install.py do, but webui already have hook mechanism and a well-designed extension should minimise the execution code on loading time, and use appropriate hooks as intended.

while this mainly affects the extensions that integrates with or dependent onother extensions, but those extensions do exist

also so even there is no conflicts within extensions also it is totally possible during loading of extension module the extension decides to modify / patch parts of web UI which is not thread safe

any possible monkey patches should use proper hooks If he want to guarantee the intended behaviour.

basically if the PR is meaged is that it will requires all extension loading to be rewritten with thread safe manner and since this is not the case in the past, an extension has to declare that they are thread safe for us to load it in parallel

it is totally possible for something to be broken to the point prevents the UI from even launching furthermore the behavior is undeterministic which basically makes debugging impossible this feature is also enabled by default which is asking for more trouble

we don't have to make perfect extensions, simply we can switch this feature on and off by setting script_loading_max_thread more than 1, and slowly migrate/fix/propose some badly designed extensions.

wkpark avatar Oct 20 '24 04:10 wkpark

but webui already have hook mechanism and a well-designed extension should minimise the execution code on loading time, and use appropriate hooks as intended. any possible monkey patches should use proper hooks If he want to guarantee the intended behaviour.

there are far more things that doesn't have a hook or callback then things that has the controlnet extension uses a bunch of patches called "hijacks"

webui is by designed expecting extensions will apply their own patches when necessary in case you don't know there's a on_script_unloaded callback, the documented use of this callback is to undo patches

we don't have to make perfect extensions, simply we can switch this feature on and off by setting script_loading_max_thread more than 1, and slowly migrate/fix/propose some badly designed extensions.

I just can't see including something that has potential of breaking webui in non-deterministic ways as a feature as a good idea

while this does indeed has potential of decreasing low time but it has a bigger potential of generating some weird issue that is practically impossible to debug leading to some unpaid developer spending 100x time that the future saves in a fruitless effort of trying to solve the issue

I see the potential troubles that this feature may cause outweigh its benefits


I forgot to mention some scripts are explicitly designed to be loaded after some other Scripts you can't even specify you wish for extension to be loaded after another extension and in general there are certain internal scripts that are must be loaded in a specific order this is already built into the current system

your PR that added thread pool basically throws the order away if you use a low worker account, you might be lucky to avoid the issues of what occur if the orders are messed up

but if you are work account is higher, you could easily experience issues like this as there's certain callbacks that the order is based on the load order this includes the ui callbackl image

you can see the extension order in the front end UI is randomized on every re-load the test is done by setting worker to 8 with 7 extentions that it has a random time sleep 0~5 sec in the path of loading of the module to simulate a worst case scenario

if I recall correctly the load the order of an extension also affects the internal parameter order that is sent to the back end when processing a generation job, means that the parameter index changes

why does this internal order matter?

currently let's say you have a web UI web page open, if you're done for some reason decide to restart the back end server assuming that they are not major changes such as enabling or disabling extensions across restart once the server is up and running the existing webpage will continue to work as if nothing has happened this is because the argument that is sent to and from the client and server has not change

but if you parallelize script module load order, this is no longer the case it is totally possible that on each restart the parameter order is different and the web page will no longer function properly

it forces you to reload a web page if the server ever reboot this is perfectly bad for multi-person usage as probably only the admin would know that the server is restarted

if you wish to simulate an issue similar to this, you could two webui webpage use one webpage to enable / disable an extention then restart the server, after the server has restarted you would use the old web page that's still thinks that the extension is disable / enable to sent a request the te server

if you're "lucky" things will break, if you're unlucky things might seem to work, in the worst case it has the potential of corrupt your settings as wrong parameters are essentially wrong values


I still stand my point unless an extension is explicitly written to be and declared itself as thread safe, we should not run it in parallel and I really don't see this happening

w-e-w avatar Oct 20 '24 06:10 w-e-w

conclusion the current implementation is objectively broken has it has the potential of breaking even vanilla web UI and the situation issue only gets worse if extensions are mixed and this is before considering extensions that patches functions

it requires much much more work to allow the modules to be loaded in parallel

w-e-w avatar Oct 20 '24 06:10 w-e-w