fzy: replace fzy.lua with the C version #15753
Open
Conversation
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
fzy.lua was added because it is less code than the C version, despite being slower. But it makes a visible difference in the new history selector, even if you don't let the history grow that huge, because it has long lines with full paths. This makes it slower than the property selector with 12k items even with fewer history entries than that, because properties are shorter strings. Even on new a CPU it causes visible lag on the first character typed with 10k history entries. With something like 100k entries it is totally unusable. Using the C version completely fixes performance. It also visibly speeds up the property list, especially on old hardware. When typing the first character in it on a 2013 CPU, Lua 5.1 fzy takes 300ms, Luajit fzy takes 130ms, while C fzy takes 10ms. It can also help with loadfile autocompletion in very large directories, or arbitrary user scripts using huge haystacks. Since this is just a copy pasted library we don't have to update and not much bigger than the Lua version, there is no reason to give up the extra performance. We copy pasted osdep/dirent-win.h which is twice as big, though more self-contained. This C version can also be exposed to Javascript too, if someone wants that. Or it was suggested to use it for --sub-auto=fuzzy.
|
Has the C version been reviewed from a security POV? Anyone tried throwing a fuzzer at it? |
|
I don't like injecting foreign C code into mpv core. At the very least it needs to be adopted to use our talloc primitives. Probably also Also this code needs to be tested in |
|
Download the artifacts for this pull request: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
fzy.lua was added because it is less code than the C version, despite being slower. But it makes a visible difference in the new history selector, even if you don't let the history grow that huge, because it has long lines with full paths. This makes it slower than the property selector with 12k items even with fewer history entries than that, because properties are shorter strings. Even on new a CPU it causes visible lag on the first character typed with 10k history entries. With something like 100k entries it is totally unusable. Using the C version completely fixes performance.
It also visibly speeds up the property list, especially on old hardware. When typing the first character in it on a 2013 CPU, Lua 5.1 fzy takes 300ms, Luajit fzy takes 130ms, while C fzy takes 10ms.
It can also help with loadfile autocompletion in very large directories, or arbitrary user scripts using huge haystacks.
Since this is just a copy pasted library we don't have to update and not much bigger than the Lua version, there is no reason to give up the extra performance. We copy pasted osdep/dirent-win.h which is twice as big, though more self-contained.
This C version can also be exposed to Javascript too, if someone wants that. Or it was suggested to use it for --sub-auto=fuzzy.