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-125196: Use PyUnicodeWriter for repr(list) #125202

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 9, 2024

Replace the private _PyUnicodeWriter with the public PyUnicodeWriter.

Replace the private _PyUnicodeWriter with the public PyUnicodeWriter.
Objects/listobject.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

Benchmark:

import pyperf
runner = pyperf.Runner()

runner.bench_func('repr([])', repr, [])
runner.bench_func('repr([1,2,3])', repr, [1, 2, 3])
list3 = ['abcdef']*10
runner.bench_func('repr(list3)', repr, list3)
list4 = ['abcdef']*50
runner.bench_func('repr(list4)', repr, list4)

Result, Python built with gcc -O3:

+----------------+---------+-----------------------+
| Benchmark      | ref     | change                |
+================+=========+=======================+
| repr([])       | 104 ns  | 98.1 ns: 1.06x faster |
+----------------+---------+-----------------------+
| repr([1,2,3])  | 534 ns  | 558 ns: 1.04x slower  |
+----------------+---------+-----------------------+
| repr(list3)    | 1.53 us | 1.59 us: 1.04x slower |
+----------------+---------+-----------------------+
| repr(list4)    | 5.36 us | 5.53 us: 1.03x slower |
+----------------+---------+-----------------------+
| Geometric mean | (ref)   | 1.01x slower          |
+----------------+---------+-----------------------+

In the worst case, it's 24 ns slower, 534 ns => 558 ns: 1.04x slower.

@serhiy-storchaka @pitrou: Do you think that it's an acceptable slowdown, under 10% slower on a microbenchmark?


error:
_PyUnicodeWriter_Dealloc(&writer);
if (writer != NULL) {
PyUnicodeWriter_Discard(writer);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not PyUnicodeWriter_Discard(NULL) be no-op?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I modified PyUnicodeWriter_Discard(writer) to do nothing if writer is NULL.

if (i > 0) {
if (_PyUnicodeWriter_WriteASCIIString(&writer, ", ", 2) < 0)
if (PyUnicodeWriter_WriteUTF8(writer, ", ", 2) < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is faster, PyUnicodeWriter_WriteUTF8(writer, ", ", 2) or two PyUnicodeWriter_WriteChar()?

This is an idle question. Even if the latter is marginally faster, it may still be more preferable to use the former. But if the difference is significant, this may be a question for optimization.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is faster, PyUnicodeWriter_WriteUTF8(writer, ", ", 2) or two PyUnicodeWriter_WriteChar()?

Two PyUnicodeWriter_WriteChar() calls is faster:

+----------------+-------------+-----------------------+
| Benchmark      | change_utf8 | change_2char          |
+================+=============+=======================+
| repr([])       | 50.2 ns     | 54.0 ns: 1.07x slower |
+----------------+-------------+-----------------------+
| repr(list3)    | 825 ns      | 802 ns: 1.03x faster  |
+----------------+-------------+-----------------------+
| repr(list4)    | 2.89 us     | 2.66 us: 1.09x faster |
+----------------+-------------+-----------------------+
| Geometric mean | (ref)       | 1.01x faster          |
+----------------+-------------+-----------------------+

@pitrou
Copy link
Member

pitrou commented Oct 9, 2024

@serhiy-storchaka @pitrou: Do you think that it's an acceptable slowdown, under 10% slower on a microbenchmark?

Definitely!

@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

Definitely!

Combined with a minor change, PR gh-125214, it's even faster :-)

@serhiy-storchaka
Copy link
Member

Combined with a minor change, PR gh-125214, it's even faster :-)

Well, than integers are not suitable for this microbenchmark, you should use something else.

@@ -13428,6 +13428,9 @@ PyUnicodeWriter_Create(Py_ssize_t length)

void PyUnicodeWriter_Discard(PyUnicodeWriter *writer)
{
if (writer == NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be documented. Please do this in a separate PR.

Also, I think it is worth to try to optimize PyUnicodeWriter_WriteUTF8() for short ASCII strings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be documented. Please do this in a separate PR.

Oh ok. I reverted the change.

@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

Updated benchmark results with CPU isolation:

vstinner@mona$ python3 -m pyperf compare_to --table ref.json change.json
+----------------+---------+-----------------------+
| Benchmark      | ref     | change                |
+================+=========+=======================+
| repr([])       | 57.2 ns | 52.2 ns: 1.09x faster |
+----------------+---------+-----------------------+
| repr([1,2,3])  | 283 ns  | 293 ns: 1.03x slower  |
+----------------+---------+-----------------------+
| repr(list3)    | 827 ns  | 807 ns: 1.02x faster  |
+----------------+---------+-----------------------+
| repr(list4)    | 2.80 us | 2.74 us: 1.02x faster |
+----------------+---------+-----------------------+
| Geometric mean | (ref)   | 1.03x faster          |
+----------------+---------+-----------------------+

Now it's only slower for repr([1,2,3]), but this row will be made way faster using gh-125214.

Well, than integers are not suitable for this microbenchmark, you should use something else.

Only repr([1,2,3]) row uses integers, list3 and list4 use strings.

@vstinner vstinner merged commit 52f70da into python:main Oct 9, 2024
35 checks passed
@vstinner vstinner deleted the writer_list branch October 9, 2024 21:56
@vstinner
Copy link
Member Author

vstinner commented Oct 9, 2024

I merged, thanks for reviews. There is apparently room for performance improvement. Let's use this change as a starting point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants