defaults.lua: add an exit() function #15235
Conversation
|
Download the artifacts for this pull request: |
guidocella
force-pushed
the
lua-exit
branch
2 times, most recently
from
6668eb2
to
e2547ca
Compare
What is the use-case for that?
Worker scripts should never exit or be reloaded, it is wasteful. They should register script-message and wait for that. See for example how gallery-thumbgen.lua is implemented.
That's why exit() is not a good way to close script. People will forget to unbind everything. Scripts should generally unbind their hooks and do nothing after. Forcefully closing their event loop is not expected by all the bookkeeping in mpv. |
guidocella
force-pushed
the
lua-exit
branch
2 times, most recently
from
0fa0c7a
to
44f2ad2
Compare
No. It's wasteful to keep them running doing nothing. Each has a scripting VM which just idles. It's nicer to launch when needed and terminate when no longer needed.
That note was added originally before mpv added that functionality (even if the commit dates show otherwise), but it's no longer true, and removed in the followup force-push. Good catch by @guidocella . |
That's literally the script we discussed in #mpv, isn't it more wasteful to always keep 6+ threads of worker scripts running? auto_profiles.lua also uses it.
Actually key bindings were already removed automatically, I updated the commits. |
Do you have profiling that would confirm this claim? Idling sleeping threads are almost free and you have hundreds if not thousands of them on your system. It is common pattern to have thread pool with worker threads waiting for a tasks to process. Even mpv has thread pool implemented. Latency of starting thread / creating client context / starting VM / parsing script each time we want to do anything will increase latency of all operations on it. |
You don't need a profiler for that. They don't consume meaningful CPU resources, but each scripting VM uses between hunderds of KB to many MB of RAM. It's wasteful to keep them running when they're no longer needed. (they do consume a bit CPU, because all mpv events reach all scripts by default, and GC runs once in a while due to that, but it is negligible)
Sure. and a very valid use case. But the system doesn't enforce keeping things in memory after they're used. The code should do that explicitly (or in our case, decide whether it wants to quit the worker scripts or keep them running). This is the same as in any other kind of environment. Except for caching purposes (which the code should decide for itself what it wants to cache), you don't keep stuff around once it's no longer needed.
This is for the script to decide. They're not required to quit the worker scripts. Launching a scripting VM is low single-digit ms at most. I'm guessing that in the case of the thumbnails script it's negligible compared to generating even a single thumnail image. |
Again, do you have any profiling to support your claims? You make a lot of statements, but none of them are backed up with evidence. I won’t discuss this further; throwing unverifiable claims back and forth doesn’t make sense. |
I tested that in the past. I measured launching a mujs JS VM in mpv about 10 years ago, and it was about ~ 4 ms (lanching the VM, parsing and running It should be faster with more modern systems. Lua should launch quicker because it has a lot less thing to init on startup (JS has many internal objects which are initialized unconditionally in mujs, like That being said, even if it was 100ms - and it's definitely definitely orders of magnitude less, it's still for the script to decide at which point pooling/cachine makes sense. Maybe it's always, maybe it's never. The script is not required to quit, and they behave the same as before. We're simply adding an option which allows them to quit, if they so decide.
Even if irrelevant because it doesn't matter how long it takes, only the "low-digit ms" needs evidence as far as I can tell - which I've given, unless you mean other things too. The other things they don't need evidence as far as I can tell. Not keeping resources around once they're no longer needed, except for caching - which the code should decide for itself what it wants to cache/pool, and for how long, is a generally correct statement without requiring any evidence to back it up. Are you actually arguing that we should not allow scripts to exit once they decide that they have nothing else to do? if yes, then just expplain why we shouldn't allow it instead of requestig evidence to things which need none. If no, then what else do you want? |
guidocella
force-pushed
the
lua-exit
branch
2 times, most recently
from
79f1c74
to
f4dd579
Compare
|
Recent docs changes LGTM. Thanks. I'll merge it a bit later if there are no objections. |
Scripts can terminate execution by setting mp.keep_running = false. Add an exit() function to wrap setting mp.keep_running and properly expose this feature. It can be used e.g. by a thumbnail script to spawn workers with load-script and then let them quit. It is not added to the mp namespace as mp.exit because that would make it look like it terminates mpv. This mirrors the exit() function which already exists in js. The note in javascript.rst about having to remove key bindings before exit is not kept because they are actually removed automatically since bf385e1 (though it was accurate when the JS backend was developed before upstreaming it).
Unsetting _G.mp_event_loop at the top level quits the script, but not within callbacks. Use the new exit() function instead. Fixes e2284fb. This actually has an edge case since e2284fb where you can add auto profiles only later with load-config-file and the script stays unloaded, but it's still reasonable to quit if mpv.conf has no conditional profiles. You could always explicitly set --load-auto-profiles=yes in this case.
It's a commonly used function so group it with the documented functions.
|
Thanks. |
commit 1: defaults.lua: add an exit() function
Scripts can terminate execution by setting mp.keep_running = false. Add an exit() function to wrap setting mp.keep_running and properly expose this feature. It can be used e.g. by a thumbnail script to spawn workers with load-script and then let them quit.
It is not added to the mp namespace as mp.exit because that would make it look like it terminates mpv.
commit 2: auto_profiles.lua: actually exit when no auto profiles are defined
Unsetting _G.mp_event_loop at the top level quits the script, but not within callbacks. Use the new exit() function instead. Fixes e2284fb.
This actually has an edge case since e2284fb where you can add auto profiles only later with load-config-file and the script stays unloaded, but it's still reasonable to quit if mpv.conf has no conditional profiles. You could always explicitly set --load-auto-profiles=yes in this case.