Repository navigation
Generalize usage of _PyUnicodeWriter for repr(obj): add _PyObject_ReprWriter() #63852
Description
| BPO | 19653 |
|---|---|
| Nosy | @pitrou, @vstinner, @ericsnowcurrently, @serhiy-storchaka |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2013-11-20.15:26:25.002>
created_at = <Date 2013-11-19.13:00:58.075>
labels = ['invalid']
title = 'Generalize usage of _PyUnicodeWriter for repr(obj): add _PyObject_ReprWriter()'
updated_at = <Date 2013-11-20.15:26:25.001>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2013-11-20.15:26:25.001>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2013-11-20.15:26:25.002>
closer = 'vstinner'
components = []
creation = <Date 2013-11-19.13:00:58.075>
creator = 'vstinner'
dependencies = []
files = ['32701']
hgrepos = []
issue_num = 19653
keywords = ['patch']
message_count = 5.0
messages = ['203371', '203459', '203461', '203491', '203492']
nosy_count = 4.0
nosy_names = ['pitrou', 'vstinner', 'eric.snow', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'not a bug'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue19653'
versions = ['Python 3.4']
Activity
vstinner commentedon Nov 19, 2013
The _PyUnicodeWriter API avoids creation of temporary Unicode strings and has very good performances to build Unicode strings with the PEP-393 (compact unicode string).
Attached patch adds a _PyObject_ReprWriter() function to avoid creation of tempory Unicode string while calling repr(obj) on containers like tuple, list or dict.
I did something similar for str%args and str.format(args).
To avoid the following code, we might add something to PyTypeObject, maybe a new tp_repr_writer field.
+ if (PyLong_CheckExact(v)) {
+ return _PyLong_FormatWriter(writer, v, 10, 0);
+ }
+ if (PyUnicode_CheckExact(v)) {
+ return _PyUnicode_ReprWriter(writer, v);
+ }
+ if (PyList_CheckExact(v)) {
+ return _PyList_ReprWriter(writer, v);
+ }
+ if (PyTuple_CheckExact(v)) {
+ return _PyTuple_ReprWriter(writer, v);
+ }
+ if (PyList_CheckExact(v)) {
+ return _PyList_ReprWriter(writer, v);
+ }
+ if (PyDict_CheckExact(v)) {
+ return _PyDict_ReprWriter(writer, v);
+ }
For example, repr(list(range(10))) ('[0, 1, 2, 3, 4, 5, 6, 7, 8, 9]') should only allocate one buffer of 37 bytes and then shink it to 30 bytes.
I guess that benchmarks are required to justify such changes.
ericsnowcurrently commentedon Nov 20, 2013
As far as I'm aware, the performance of __repr__() for any object is not much of a concern. Repr is mostly for debugging and interactive use, so it's already fast/efficient enough for the target consumers: us. :) Making __repr__() easier to write or maintain is worth it as long as benefit outweighs the cost of the churn.
serhiy-storchaka commentedon Nov 20, 2013
I supported patches for repr(list), repr(tuple) and repr(dict) because they made the code shorter and cleaner. But this patch is more questionable.
pitrou commentedon Nov 20, 2013
Why would you want to speed up repr()? I'm -1 unless there's an use case.
vstinner commentedon Nov 20, 2013
After writing the patch, I realized that it adds a lot of new functions and add more code. Then I asked myself if repr() is really an important function or not :-)
3 other developers ask the same question, so it's probably better to reject this huge patch :-)