-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add C accelerator for fnmatch
#121445
Comments
Could you run benchmarks for more optimized python version? normcase = os.path.normcase
os_path = os.path
def filter(names, pat):
pat = normcase(pat)
match = _compile_pattern(pat)
if os_path is posixpath:
return list(filter(match, names))
else:
ncased = map(match, map(normcase, names))
return list(itl.compress(names, ncased)) Also, don't run res = list(filter(match, names)) |
The 'filter' here is the built-in filter, not the fnmatch's filter. So I must use Also, for non-posixpath paths, I will not use them since the implementation is only available on Windows platforms. I'll first try to make the C implementation compile on the CI/CD (I can run it fine on my laptop but I didn't know how to correctly update the entire ecosystem, but I think I managed to today). |
What a blunder. I did not thought that: #include <fnmatch.h>
#include <stdio.h>
void main() {
int res = fnmatch("[a-z]", "a", 0);
printf("match: %s\n", res == FNM_NOMATCH ? "no" : "yes"); // 'yes'
res = fnmatch("[^-`]", "~", 0);
printf("match: %s\n", res == FNM_NOMATCH ? "no" : "yes"); // 'yes'
} but in Python... EDIT: let's forget about |
fnmatch.filter
& co.fnmatch
& co.
fnmatch
& co.fnmatch
It can be run on Unix too, but is only executed on windows. You can test it on Unix and it will be a good proxy for it. |
In the end everything is the same as in Python. I'll regenerate the benchmarks as well. |
Implementation of ideas from #122289 at the C level gave even better performances in a release (non-PGO) build:
|
And by implementing more ideas, I get in PGO builds
We are going very fast now! |
One question. Is the implementation actually 2K lines long? |
Yes, but there are a lot of comments. So it's probably 1k long I think (in those 2k, there are ~400 that are clinic, 150 that are tests). |
Performance results look good. Hopefully code can be simplified though. |
The code could be simplified by removing the implementation of |
That sounds sensible. It doesn't seem there is going to be any significant performance improvements from having any other functions written in C. |
Yes, I'm working on an alternate branch that removes those bits. We can always re-add them later but yes, the performances are really not that impressive (10% gain is still good but not that good). |
Following #123181 (comment) and #123181 (comment), I decided to close this feature. While the improvements are significant, they will not necessarily be that significant in production. I suggest we rather use #122289 which already bring an almost 2x improvement and is a lot easier to maintain. |
Enhancement
By adding a C accelerator, it is possible to improve the running time of
fnmatch.filter
andfnmatch.translate
(the previous proposal is no longer correct sincefnmatch(3)
does not produce the same results as Python) by 1.1x and 6.5x respectively.Machine specs:
fnmatch.translate
Without
--with-pydebug
With
--enable-optimizations
Benchmark script
fnmatch.filter
(POSIX)Here are the true timings for using
fnmatch.filter
. I think we can change the Python implementation as well but I did not bother doing it in the PR (too lazy...). By the way, the timings are for POSIX platforms. On non-posix platformsos.path.normcase
is called on every name before performing the match, but if the POSIX implementation is faster, I doubt the non-posix one would be slower (the code is essentially the same). I did not include the DEBUG builds since they are not really of interest to us.Without
--with-pydebug
With
--enable-optimizations
Benchmark script
Linked PRs
fnmatch
#121446fnmatch.translate
#123181The text was updated successfully, but these errors were encountered: