Skip to content
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

gh-121445: C implementation for fnmatch.translate #123181

Closed

Conversation

picnixz
Copy link
Contributor

@picnixz picnixz commented Aug 20, 2024

Smaller PR compared to #121446 (I was too lazy for making a new branch so it's a checkout from that other PR). This only adds the C acceleration part for fnmatch.translate. The improvements brought by this PR are quite good:

Release

+------------------------------------------+-----------------------+-----------------------+
| Benchmark                                | fnmatch-translate-ref | fnmatch-translate-c   |
+==========================================+=======================+=======================+
| abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!]  | 6.09 us               | 1.26 us: 4.84x faster |
+------------------------------------------+-----------------------+-----------------------+
| !abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!] | 6.40 us               | 1.29 us: 4.97x faster |
+------------------------------------------+-----------------------+-----------------------+
| a**?**cd**?**??k***                      | 2.17 us               | 303 ns: 7.16x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a/**/b/**/c                              | 2.00 us               | 286 ns: 7.00x faster  |
+------------------------------------------+-----------------------+-----------------------+
| man/man1/bash.1                          | 3.04 us               | 459 ns: 6.63x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a*b*c*d*e*f*g*h*i*j*k*l*m*n*o**          | 5.14 us               | 685 ns: 7.50x faster  |
+------------------------------------------+-----------------------+-----------------------+
| Geometric mean                           | (ref)                 | 6.26x faster          |
+------------------------------------------+-----------------------+-----------------------+

PGO

+------------------------------------------+-----------------------+-----------------------+
| Benchmark                                | fnmatch-translate-ref | fnmatch-translate-c   |
+==========================================+=======================+=======================+
| abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!]  | 6.15 us               | 1.21 us: 5.07x faster |
+------------------------------------------+-----------------------+-----------------------+
| !abc/[!]b-ac-z9-1]/def/\*?/*/**c/?*[][!] | 6.45 us               | 1.24 us: 5.19x faster |
+------------------------------------------+-----------------------+-----------------------+
| a**?**cd**?**??k***                      | 2.15 us               | 287 ns: 7.49x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a/**/b/**/c                              | 1.94 us               | 269 ns: 7.21x faster  |
+------------------------------------------+-----------------------+-----------------------+
| man/man1/bash.1                          | 3.00 us               | 427 ns: 7.03x faster  |
+------------------------------------------+-----------------------+-----------------------+
| a*b*c*d*e*f*g*h*i*j*k*l*m*n*o**          | 5.07 us               | 636 ns: 7.98x faster  |
+------------------------------------------+-----------------------+-----------------------+
| Geometric mean                           | (ref)                 | 6.56x faster          |
+------------------------------------------+-----------------------+-----------------------+

@barneygale
Copy link
Contributor

barneygale commented Aug 20, 2024

Is this worth the added code complexity? Most users interact with translate() via one of the other functions (fnmatch(), fnmatchcase(), filter()), and those functions call translate() via an LRU cache, so the speed of the Python implementation matters less.

@picnixz
Copy link
Contributor Author

picnixz commented Aug 21, 2024

You're right on this matter. To be honest, I wanted to improve fnmatch in C at that time because it was a simple module and it was also a way to familiarize myself with the C API and its internals (this also helped in writing python/devguide#1350). However, I don't mind not having this feature and understand your concerns (though I wouldn't imagine that we would gain a factor 6.5x in speed...).

Now, as you could see in the other PR, the improvements brought by fnmatch.filter are marginal so we don't really have a user gain, so I won't push for this feature (I'm satisfied with what I learned at least!)

@erlend-aasland
Copy link
Contributor

Based on the discussion above, I believe we can close this PR?

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Aug 23, 2024
@picnixz
Copy link
Contributor Author

picnixz commented Aug 23, 2024

Oh yes. We can close it. I'll keep the branch on my side if I ever want to ressurect it though but yes. Sorry for forgetting about it.

@picnixz picnixz closed this Aug 23, 2024
@picnixz picnixz removed awaiting review pending The issue will be closed if no feedback is provided labels Aug 23, 2024
@picnixz picnixz removed request for a team, corona10 and erlend-aasland August 23, 2024 08:36
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.

3 participants