Skip to content

player/lua/defaults: add LuaJIT profiling support#17785

Open
N-R-K wants to merge 1 commit intompv-player:masterfrom
N-R-K:nrk/luajit-profiler
Open

player/lua/defaults: add LuaJIT profiling support#17785
N-R-K wants to merge 1 commit intompv-player:masterfrom
N-R-K:nrk/luajit-profiler

Conversation

@N-R-K
Copy link
Copy Markdown
Contributor

@N-R-K N-R-K commented Apr 20, 2026

gives us a way to properly profile the lua scripts to get some verifiable way to test which "optimizations" work and which don't.

currently only supports LuaJIT but the cli option is generically named in case support for profiling other scripts is added in the future.

@N-R-K N-R-K force-pushed the nrk/luajit-profiler branch from 66167cb to 48d0617 Compare April 20, 2026 21:27
@N-R-K
Copy link
Copy Markdown
Contributor Author

N-R-K commented Apr 20, 2026

Pretty rudimentary right now. Just dumps to a file/stdout. Maybe makes sense to display it in somewhere in the stat page? Open to suggestion on how the interface should be like.

@kasper93
Copy link
Copy Markdown
Member

Just dumps to a file/stdout.

I think we could just print it into the log.

Maybe makes sense to display it in somewhere in the stat page?

I think it's too much info to fit info stat page. Technically with some preprocessing we could show some info. Similar to perf top, but not sure it's really that critical.

@N-R-K N-R-K force-pushed the nrk/luajit-profiler branch 3 times, most recently from e0e4a7c to 7090de5 Compare April 21, 2026 13:48
@N-R-K N-R-K marked this pull request as ready for review April 21, 2026 13:48
@N-R-K N-R-K changed the title [WIP] player/lua/defaults: add LuaJIT profiling support player/lua/defaults: add LuaJIT profiling support Apr 21, 2026
@kasper93 kasper93 added priority:needs-author-feedback The original author of the issue/PR needs to come back and respond to something labels Apr 22, 2026
@N-R-K
Copy link
Copy Markdown
Contributor Author

N-R-K commented Apr 22, 2026

image

What feedback is needed exactly? I don't see any questions. And the patch was updated to print it via mpv's logging system by default if no path was given like requested.

gives us a way to properly profile the lua scripts to get some
verifiable way to test which "optimizations" work and which
don't.

currently only supports LuaJIT but the cli option is generically
named in case support for profiling other scripts is added in
the future.
@N-R-K N-R-K force-pushed the nrk/luajit-profiler branch from 7090de5 to 5718675 Compare April 22, 2026 12:08
@kasper93
Copy link
Copy Markdown
Member

image

What feedback is needed exactly? I don't see any questions. And the patch was updated to print it via mpv's logging system by default if no path was given like requested.

Keep it simple, you can grep the log if you need profile in another file, there is no need to NIH file writing.

@kasper93 kasper93 removed the priority:needs-author-feedback The original author of the issue/PR needs to come back and respond to something label Apr 22, 2026
@N-R-K
Copy link
Copy Markdown
Contributor Author

N-R-K commented Apr 22, 2026

there is no need to NIH file writing.

It's just standard lua io, what's NIH here?

you can grep the log if you need profile in another file

Very common case is to have a base profile and then make change and collect another profile. Yes, you can just grep the log and extract it manually, but if we're going to add a cli option it should at least make the common cases easy to handle. Otherwise, you can also argue "KISS, just let users do their own luajit markup themselves".

@kasper93
Copy link
Copy Markdown
Member

kasper93 commented Apr 22, 2026

Otherwise, you can also argue "KISS, just let users do their own luajit markup themselves".

I agree with you. If the main feature of this integration is saving to external text file, than indeed it's best to not include it in the mpv core.

@kasper93
Copy link
Copy Markdown
Member

Also if you prefer more native interface, you could bring up the changes from @avih d708977

@N-R-K
Copy link
Copy Markdown
Contributor Author

N-R-K commented Apr 22, 2026

If the main feature of this integration is saving to external text file

Interesting. The ~8 loc saving the profiling report to a file is the main feature and not the other ~100 loc to collect and format the profiling information?

indeed it's best to not include it in the mpv core

Almost entirety of the added code lives in defaults.lua, not mpv core. Only thing added to mpv core was the cli flag. If that's a problem, maybe some kind of script-opt can be used instead.

@kasper93
Copy link
Copy Markdown
Member

Interesting. The ~8 loc saving the profiling report to a file is the main feature and not the other ~100 loc to collect and format the profiling information?

Out of the 106 added lines in default.lua, 44 are only related to saving the profiling report to a file. Don't be disingenuous by saying it's ~8 loc. It's not about loc.

I noticed that almost half of the PR is about saving file, and I suggested to keep it simple and log it through our existing log code, that can also save to file.

It can be 1000 loc, if there is a value it that, you can draw graphs, flame graphs, whatever, I don't mind.

Almost entirety of the added code lives in defaults.lua, not mpv core.

default.lua is very much mpv core, it's embedded and used for every script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants