osc.lua: extract icon_font and icons variables #15645
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove support for puc lua?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we really shouldn't.
luajit is a project with a bus-factor of 1, highly non-hackable, and way way more complex code than puc-lua (5.1 or 5.2).
Such a core part of mpv (lua scripting) cannot depend on one person to release sources which build correctly on every platform which mpv supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be in favor of dropping support for Lua 5.1 because it makes you waste a lot of time converting unicode code points (I have the same issue in my LRC script) and the performance improvement of LuaJIT is nice for fuzzy finding, it will matter even more once history search is merged since that can grow infinitely. If LuaJIT is abandoned we should switch to Lua 5.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely you don't need to do that more than once, because at least this comment was about literal codepoints embedded in literal strings as part of the script soure code, and that wouldn't change with each iteration of any loop. And if you do and can't avoid it and have to do whatever a lot, then write the slow part in C.
Using luajit is not a magic solution. because even if it's 5x faster with luajit, you run it on a RPI and it becomes x5 slower again.
It's a script, it can't be expected to perform like C code. But before that, do ensure your whatever algorithm is efficient, because typically it should be enough to gain the speed you prefer.
Also, IIRC 5.1 and 5.2 perform similarly across different use cases, from my own local tests in the past (sometimes one is faster than the other, but it's not necessarily always 5.2 which is faster, and largely they're similar).
And it's better to support more than once version, so I think we should keep both.
Something which can grow indefinitely doesn't sound like a great idea to me. Surely you can drop some years old historical commands which you once issued accidentally. So pick a reasonable limit and a useful search algorithm, and it should be OK, and if not, again, write it in C, or drop this feature.
It's a script. Don't push it beyond reasonable limits.
And one can't expect minimum absolute performance (e.g. "this must finish in under 5ms"), because it can run on a wide range of processors, and an inefficient algorithm will remain slow regardess of the processor (or scripting VM), especially as the input grows.
So keep the runtime linear as much as possible with the input size, or at most log-linear, ensure each iteration is reasonably efficient, and then ensure the input size is also within reasonable limits.
That's how code works. When the input becomes big, the algorithm and implementation matter more than the absolute performance of each line of code, and limits should be put in place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was talking about the watch history. It is up to users when to clear that. I did suggest integrating the C version of fuzzy search iniitally but it was rejected in favor of a Lua script because that was smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, these were my measurements last time I tested it for calling fzy when typing 1 letter in the g-r selector (10k properties):
lua 5.1: 0.166s
luajit: 0.08s
C: 0.005s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I got that right, luajit ended up 2x faster than 5.1? IMO that's a pretty small diffrence in the grand scheme of things. Definitely not enough to consider dropping 5.1. (yes, I know luajit can be much faster than 5.1 in some cases, but you always need to look at the overall gains, and not through the needle hole, and in the big pictures there are typically many more factors than how fast one function runs).
Also, I didn't look at the code, but there might still be room for further improvements. Usually there is, but not always enough of it to warrant the additional effort and possible complexity, but sometimes a small change is enough to make a big difference which is worth it.