GeezerVison

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

cc @serhiy-storchaka

To make bot happy:

I have made the requested changes; please review again

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

Retriggering, this failure is listed in #97983

Benches

Old Version (main)

» pyperf timeit '"abcabd".count("ab")'
.....................
Mean +- std dev: 495 ns +- 7 ns

» pyperf timeit '":) тест 漢字".count("т")'
.....................
Mean +- std dev: 453 ns +- 13 ns

New version (issue-97982)

New optimization:

» pyperf timeit '"abcabd".count("ab")'
.....................
Mean +- std dev: 459 ns +- 9 ns

Old path:

» pyperf timeit '":) тест 漢字".count("т")'
.....................
Mean +- std dev: 457 ns +- 16 ns

Merging two functions which are basically the same is a good thing.

This now makes unicode_count as optimized as PyUnicode_Count. There was a missing optimization, see c3cec78

But I'm not convinced that asciilib_count() is different than ucs1lib_count(). It seems like the code is exactly the same, it doesn't see to use the property that the string is ASCII only.

» pyperf timeit '"abcabd".count("ab")'

• before: Mean +- std dev: 495 ns +- 7 ns
• after: Mean +- std dev: 459 ns +- 9 ns

I'm not convinced. You didn't explain how you built Python. Did you enable LTO and/or PGO? Which is your OS? Which compiler?

But I'm not convinced that asciilib_count() is different than ucs1lib_count().

I ran a benchmark: there is no difference. I would suggest to remove asciilib_count(). But merging the PyUnicode_Count() and unicode_count() is still interesting.

Micro-benchmark:

Mean +- std dev: [ref] 64.1 ns +- 0.1 ns -> [change] 62.3 ns +- 0.2 ns: 1.03x faster

Command:

$ ./configure --with-lto && make clean && make 
$ ./python -m venv env && ./env/bin/python -m pip install pyperf
$ ./env/bin/python -m pyperf timeit -o ../change.json -v '"abcabd".count("ab")'

Python built with LTO (without PGO), on Linux without CPU pinning. The change:

diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c
index bd169ed714..fd786fa3db 100644
--- a/Objects/unicodeobject.c
+++ b/Objects/unicodeobject.c
@@ -10885,10 +10885,16 @@ unicode_count(PyObject *self, PyObject *args)
     }
     switch (kind1) {
     case PyUnicode_1BYTE_KIND:
-        iresult = ucs1lib_count(
-            ((const Py_UCS1*)buf1) + start, end - start,
-            buf2, len2, PY_SSIZE_T_MAX
-            );
+        if (PyUnicode_IS_ASCII(self) && PyUnicode_IS_ASCII(substring))
+            iresult = asciilib_count(
+                ((const Py_UCS1*)buf1) + start, end - start,
+                buf2, len2, PY_SSIZE_T_MAX
+                );
+        else
+            iresult = ucs1lib_count(
+                ((const Py_UCS1*)buf1) + start, end - start,
+                buf2, len2, PY_SSIZE_T_MAX
+                );
         break;
     case PyUnicode_2BYTE_KIND:
         iresult = ucs2lib_count(

I ran a benchmark: there is no difference. I would suggest to remove asciilib_count().

I wrote PR #98164 for that.

If asciilib_count and ucs1lib_count are the same, why there is a difference in #98025 (comment)?

If asciilib_count and ucs1lib_count are the same, why there is a difference in #98025 (comment)?

These timings are about 500 ns, whereas I got timings about 60 ns. Maybe the benchmark with 500 ns was run on Python built in debug mode? As I wrote, I would like to know more about this benchmark: OS name and version, compiler name and options, how was Python built (debug mode or not? etc.), ...

I remove asciilib_count() function in commit df3a6d9.

Sorry for the long reply. Here's my initial setup:

• MacOS X 10.14,
• Python was built with ./configure --with-pydebug && make -j
• CC.version: Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/c++/4.2.1

Now, let's redo that once again with the exact same steps as @vstinner did.

commit before df3a6d9

The actual commit I am testing is: 7ec2e27

Steps:

1. ./configure --with-lto && make clean && make
2. ./python.exe -m venv env && ./env/bin/python.exe -m pip install pyperf
3. ./env/bin/python.exe -m pyperf timeit -v '"abcabd".count("ab")'

Output:

Mean +- std dev: 156 ns +- 6 ns

Changed

Change is taken from #98025 (comment)

diff --git a/Objects/unicodeobject.c b/Objects/unicodeobject.c
index bd169ed714..fd786fa3db 100644
--- a/Objects/unicodeobject.c
+++ b/Objects/unicodeobject.c
@@ -10885,10 +10885,16 @@ unicode_count(PyObject *self, PyObject *args)
     }
     switch (kind1) {
     case PyUnicode_1BYTE_KIND:
-        iresult = ucs1lib_count(
-            ((const Py_UCS1*)buf1) + start, end - start,
-            buf2, len2, PY_SSIZE_T_MAX
-            );
+        if (PyUnicode_IS_ASCII(self) && PyUnicode_IS_ASCII(substring))
+            iresult = asciilib_count(
+                ((const Py_UCS1*)buf1) + start, end - start,
+                buf2, len2, PY_SSIZE_T_MAX
+                );
+        else
+            iresult = ucs1lib_count(
+                ((const Py_UCS1*)buf1) + start, end - start,
+                buf2, len2, PY_SSIZE_T_MAX
+                );
         break;
     case PyUnicode_2BYTE_KIND:
         iresult = ucs2lib_count(

Exactly the same commands:

Mean +- std dev: 155 ns +- 8 ns

On your new benchmark, there is now basically the same performance:

• Before: Mean +- std dev: 156 ns +- 6 ns
• After: Mean +- std dev: 155 ns +- 8 ns

For me, the difference of 1 ns is just pure noise. It confirms that the change has no impact on performance. That's why I removed asciilib_count().

But again, please rebase your PR on the main branch and update it to move code which is in a common in unicode_count_impl() and try to use anylib_count(). Also, your tests are valuable! Just omit any mention on performance impact ;-)

Will do, thank you :)

Merged, thank you :-)

Thank you, I've learned quite a lot while working on this :)

unicodeobject.c is a giant beast who deserves some love.