gh-125196: Use PyUnicodeWriter for repr(list) by vstinner · Pull Request #125202 · python/cpython · GitHub

Replace the private _PyUnicodeWriter with the public PyUnicodeWriter.

Issue: Use the public PyUnicodeWriter API #125196

Replace the private _PyUnicodeWriter with the public PyUnicodeWriter.

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?

Should not PyUnicodeWriter_Discard(NULL) be no-op?

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

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.

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          |
+----------------+-------------+-----------------------+

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

Definitely!

Definitely!

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

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.

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.

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

Oh ok. I reverted the change.

This reverts commit 8b33b0e.

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.

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

pythongh-125196 : Use PyUnicodeWriter for repr(list)

8c33f91

Replace the private _PyUnicodeWriter with the public PyUnicodeWriter.

vstinner added the skip news label Oct 9, 2024

bedevere-app Bot mentioned this pull request Oct 9, 2024

Use the public PyUnicodeWriter API #125196

Closed

bedevere-app Bot added the awaiting core review label Oct 9, 2024

kumaraditya303 reviewed Oct 9, 2024

View reviewed changes

Comment thread Objects/listobject.c Outdated

Fix error handling

5fa3466

serhiy-storchaka reviewed Oct 9, 2024

View reviewed changes

vstinner added 2 commits October 9, 2024 22:34

Avoid PyUnicodeWriter_WriteUTF8()

0273017

PyUnicodeWriter_Discard(NULL) does nothing

8b33b0e

serhiy-storchaka reviewed Oct 9, 2024

View reviewed changes

vstinner mentioned this pull request Oct 9, 2024

gh-125196: Add fast-path for int in PyUnicodeWriter_WriteStr() #125214

Merged

Revert "PyUnicodeWriter_Discard(NULL) does nothing"

9c910b5

This reverts commit 8b33b0e.

vstinner merged commit 52f70da into python:main Oct 9, 2024

vstinner deleted the writer_list branch October 9, 2024 21:56

bedevere-app Bot removed the awaiting core review label Oct 9, 2024

               error:
-                  _PyUnicodeWriter_Dealloc(&writer);
+                  if (writer != NULL) {
+                      PyUnicodeWriter_Discard(writer);

+                  for (Py_ssize_t i = 0; i < Py_SIZE(v); ++i) {
                       if (i > 0) {
-                          if (_PyUnicodeWriter_WriteASCIIString(&writer, ", ", 2) < 0)
+                          if (PyUnicodeWriter_WriteUTF8(writer, ", ", 2) < 0) {

               void PyUnicodeWriter_Discard(PyUnicodeWriter *writer)
               {
+                  if (writer == NULL) {

gh-125196: Use PyUnicodeWriter for repr(list) by vstinner · Pull Request #125202 · python/cpython · GitHub

Repository navigation

gh-125196: Use PyUnicodeWriter for repr(list) - #125202

gh-125196: Use PyUnicodeWriter for repr(list)#125202vstinner merged 5 commits intopython:mainpython/cpython:mainfrom vstinner:writer_listvstinner/cpython:writer_listCopy head branch name to clipboard

Conversation

vstinner commented Oct 9, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

vstinner commented Oct 9, 2024

Uh oh!

serhiy-storchaka Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

pitrou commented Oct 9, 2024

Uh oh!

vstinner commented Oct 9, 2024

Uh oh!

serhiy-storchaka commented Oct 9, 2024

Uh oh!

serhiy-storchaka Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Oct 9, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gh-125196: Use PyUnicodeWriter for repr(list)#125202
vstinner merged 5 commits into
python:mainfrom
vstinner:writer_list

vstinner commented Oct 9, 2024 •
edited by bedevere-app Bot
Loading

vstinner commented Oct 9, 2024 •
edited
Loading