-
-
Notifications
You must be signed in to change notification settings - Fork 29.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
gh-118184: Support tuples for find
, index
, rfind
& rindex
#119501
Conversation
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.
Fix bytes & bytearray test
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.
Fix space
Could someone run the benchmark on Linux? I believe it will make it faster in all cases (at least for relatively small strings). |
What about |
EDIT: 2ns faster: script# find_tuple.sh
echo find && main/python.exe -m timeit "'foobar'.find('foo')" && find-tuple/python.exe -m timeit "'foobar'.find('foo')"
echo rfind && main/python.exe -m timeit "'foobar'.rfind('foo')" && find-tuple/python.exe -m timeit "'foobar'.rfind('foo')"
|
@nineteendo There is an issue that has not been addressed in the discussion: https://discuss.python.org/t/add-tuple-support-to-more-str-functions/50628/66
|
|
I'm going to try to improve the mixed case. |
Could someone run the benchmark on Linux? |
Adding do-not-merge, since the linked issue is closed as wont-implement. |
@serhiy-storchaka all your issues are now addressed. I don't like the way the if (heap_subs) {
for (Py_ssize_t i = 0; i < subs_len; i++) {
PyMem_Free((void *)heap_subs[i]);
}
} Could you please review again? If you think the code is too complex, remember you asked for this. I wanted to keep this as simple as possible, but I have been repeatedly asked to further optimise it. |
Reminding the reviewers that there are mixed opinions regarding adding this feature at all (hence the linked issue closed as wont-implement); AFAIK, no core dev has expressed immense support of the idea. |
Ideally, this PoC should be kept at your fork; the CPython repo is not the place for experimentation. Consider closing this PR and continue working on it on your fork. |
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 you think the code is too complex, remember you asked for this. I wanted to keep this as simple as possible, but I have been repeatedly asked to further optimise it.
I did not ask for this. I only pointed out that the current version was not optimal and did repeatedly unnecessary things. If the simpler version be merged, we would spend months or years on optimizing it. I given some hints about how it could be optimized, but predicted that the result will be complex.
I am not enthusiastic about this feature, because, on one hand, it is not algorithmically optimal (the optimal algorithm needs the costly preparation step, the set of needles should be "compiled" before use), and on other hand, it is too complex for "practicality beats purity".
Costly it is indeed. The only area where it's faster is where the performance wasn't needed (the worst case)! find chars best case - 3.26x slower
find chars mixed case - 2.71x slower
find chars worst case - 1.22x faster
find subs best case - 3.45x slower
find subs mixed case - 1.21x slower
find subs worst case - no difference
find many prefixes - 1.02x faster
find many infixes - 1.03x faster
rfind chars best case - 3x slower
rfind chars mixed case - 1.25x slower
rfind chars worst case - no difference
rfind subs best case - 2.95x slower
rfind subs mixed case - 1.23x slower
rfind subs worst case - no difference
rfind many suffixes - 1.02x faster
rfind many infixes - 1.03x faster
|
It looks indeed like this is not going anywhere. I suggest you continue your experiments on your own fork. If you manage to get an improved version up and running, try first to gain traction for the feature on Discord, before you create a new issue/PR. So far, no core dev is super enthusiastic about this, which means that this PR (and any other like it) is only a waste of CI and review resources. |
I am sorry, but yes, without support of at least one of core developers it is a waste of your and our time. I worked on similar code, so I can estimate how much it could cost. I could be wrong, and I'd like to be wrong in this case, but it seems that at this stage the cost/benefit ratio is too high. Your current code likely has bugs, and fixing them can make it even more complicated. It has a potential for simplification (the forward and the backward loops can be merged), but you need to understand the code from top to bottom to make it simpler and efficient, and this may be not enough. I hope you learned something new and can write better code from beginning next time and better estimate the cost of future changes. Raw memory management is relatively slow. In this case you can use an array of constants size allocated on the stack for small number of needles. It is also more efficient to allocate a single buffer in dynamic memory than several buffers. Keeping a reference to the object providing a buffer is not enough, for example the |
I have an improved version here using dynamic chunk sizes: nineteendo#2. It's now the fastest algorithm in the benchmark for cases where you wouldn't use regex (if you ignore that
That really didn't feel right to me. It's a lot of code for a very small improvement in the worst case. While the new pull request destroys this strategy using a lot less code. |
From @pfmoore on Discourse:
Benchmark for 1,000,000 characters
script
find chars best case - 2.33x faster
find chars mixed case - 1.80x faster
find chars worst case - 1.69x slower
find subs best case - 2.93x faster
find subs mixed case - 3.75x slower
find subs worst case - 1.04x slower
find many prefixes - 56.3x slower
find many infixes - 5.69x slower
rfind chars best case - 1.24x faster
rfind chars mixed case - 6.16x slower
rfind chars worst case - 1.06x slower
rfind subs best case - 2.41x faster
rfind subs mixed case - 2.26x slower
rfind subs worst case - 1.04x slower
rfind many suffixes - 54.8x slower
rfind many infixes - 2.60x slower
Old benchmark on Ubuntu
expand
📚 Documentation preview 📚: https://cpython-previews--119501.org.readthedocs.build/