Issue19512
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2013-11-06 17:05 by vstinner, last changed 2022-04-11 14:57 by admin. This issue is now closed.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
pysys_getobjectid.patch | vstinner, 2013-11-06 17:08 | review |
Messages (23) | |||
---|---|---|---|
msg202273 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 17:05 | |
In interactive mode, when I run python in gdb, I see that PyUnicode_DecodeUTF8Stateful() is called a lot of times. Calls come from PyDict_GetItemString() or PySys_GetObject() for example. Allocating a temporary Unicode string and decode a byte string from UTF-8 is inefficient: the memory allocator is stressed and the byte string is decoded at each call. I propose to reuse the _Py_IDENTIFIER API in most common places to limit calls to the memory allocator and to PyUnicode_DecodeUTF8Stateful(). |
|||
msg202275 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 17:08 | |
pysys_getobjectid.patch: - add _PySys_GetObjectId() and _PyDict_GetItemId() functions - add global identifiers for most common strings: "argv", "path", "stdin", "stdout", "stderr" - use these new functions and identifiers |
|||
msg202277 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2013-11-06 17:29 | |
PySys_GetObject() is called with followed literal strings: argv, displayhook, excepthook, modules, path, path_hooks, path_importer_cache, ps1, ps2, stderr, stdin, stdout, tracebacklimit. PyDict_GetItemString() is called with followed literal strings: __abstractmethods__, __builtins__, __file__, __loader__, __module__, __name__, __warningregistry__, _abstract_, _argtypes_, _errcheck_, _fields_, _flags_, _iterdump, _needs_com_addref_, _restype_, _type_, builtins, decimal_point, default_int_handler, displayhook, excepthook, fillvalue, grouping, imp, metaclass, options, sys, thousands_sep. Are any of these calls performance critical? |
|||
msg202278 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 17:42 | |
> Are any of these calls performance critical? I'm trying to focus on the interactive interpreter. I didn't touch literal strings used once, for example at module initialization. Well, if it doesn't make the code much uglier or much slower, it's maybe not a big deal to replace all string literals with identifiers. |
|||
msg202279 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-06 17:45 | |
New changeset a2f42d57b91d by Victor Stinner in branch 'default': Issue #19512: sys_displayhook() now uses an identifier for "builtins" http://hg.python.org/cpython/rev/a2f42d57b91d New changeset 55517661a053 by Victor Stinner in branch 'default': Issue #19512: _print_total_refs() now uses an identifier to get "showrefcount" http://hg.python.org/cpython/rev/55517661a053 New changeset af822a6c9faf by Victor Stinner in branch 'default': Issue #19512: Add PyRun_InteractiveOneObject() function http://hg.python.org/cpython/rev/af822a6c9faf |
|||
msg202280 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 17:47 | |
Oh, by the way, identifiers have a nice side effect: they are interned, and so dict lookup should be faster. |
|||
msg202281 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-06 18:06 | |
New changeset 8a6a920d8eae by Victor Stinner in branch 'default': Issue #19512: Py_ReprEnter() and Py_ReprLeave() now use an identifier for the http://hg.python.org/cpython/rev/8a6a920d8eae New changeset 69071054b42f by Victor Stinner in branch 'default': Issue #19512: Add a new _PyDict_DelItemId() function, similar to http://hg.python.org/cpython/rev/69071054b42f New changeset 862a62e61553 by Victor Stinner in branch 'default': Issue #19512: type_abstractmethods() and type_set_abstractmethods() now use an http://hg.python.org/cpython/rev/862a62e61553 New changeset e5476ecb8b57 by Victor Stinner in branch 'default': Issue #19512: eval() and exec() now use an identifier for "__builtins__" string http://hg.python.org/cpython/rev/e5476ecb8b57 |
|||
msg202283 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2013-11-06 18:51 | |
I don't think these changes are required. The interactive interpreter is not a bottleneck. And definitely adding new public functions to API needs more discussion. |
|||
msg202288 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 21:10 | |
> I don't think these changes are required. The interactive interpreter is not a bottleneck. What is the problem with these changes? Identifiers have different advantages. Errors become more unlikely because objects are only initialized once, near startup. So it put also less pressure on code handling errors :) (it is usually the least tested part of the code) > And definitely adding new public functions to API needs more discussion. You mean for PyRun_InteractiveOneObject()? Oh, it can be made private, but what is the problem of adding yet another PyRun_Interactive*() function? There are already a lot of them :-) I also worked hard to support unencodable filenames: using char*, you cannot support arbitrary Unicode filename on Windows. That's why a added many various functions with "Object" suffix. Some examples: PyWarn_ExplicitObject(), PyParser_ParseStringObject(), PyImport_AddModuleObject(), etc. Some users complained that they were not able to run Python scripts on Windows with unencodable filenames (like russian characters on an english setup). I can try to find the related issues. |
|||
msg202291 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-06 21:46 | |
New changeset 5e402c16a74c by Victor Stinner in branch 'default': Issue #19512: Add _PySys_GetObjectId() and _PySys_SetObjectId() functions http://hg.python.org/cpython/rev/5e402c16a74c New changeset cca13dd603a9 by Victor Stinner in branch 'default': Issue #19512: PRINT_EXPR bytecode now uses an identifier to get sys.displayhook http://hg.python.org/cpython/rev/cca13dd603a9 New changeset 6348764bacdd by Victor Stinner in branch 'default': Issue #19512: pickle now uses an identifier to only create the Unicode string http://hg.python.org/cpython/rev/6348764bacdd New changeset 954167ce92a3 by Victor Stinner in branch 'default': Issue #19512: add some common identifiers to only create common strings once, http://hg.python.org/cpython/rev/954167ce92a3 |
|||
msg202293 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 22:43 | |
Another problem is that PyUnicode_FromString() failure is not handled correctly in some cases. PyUnicode_FromString() can fail because an decoder error, but also because of a MemoryError. For example, PyDict_GetItemString() returns NULL as if the entry does not exist if PyUnicode_FromString() failed :-( --- PyObject * PyDict_GetItemString(PyObject *v, const char *key) { PyObject *kv, *rv; kv = PyUnicode_FromString(key); if (kv == NULL) { PyErr_Clear(); return NULL; } rv = PyDict_GetItem(v, kv); Py_DECREF(kv); return rv; } --- While working on failmalloc issues (#18048, #19437), I found some places where MemoryError caused tricky bugs because of this. Example of such issue: --- changeset: 84684:af18829a7754 user: Victor Stinner <victor.stinner@gmail.com> date: Wed Jul 17 01:22:45 2013 +0200 files: Objects/structseq.c Python/pythonrun.c description: Close #18469: Replace PyDict_GetItemString() with _PyDict_GetItemId() in structseq.c _PyDict_GetItemId() is more efficient: it only builds the Unicode string once. Identifiers (dictionary keys) are now created at Python initialization, and if the creation failed, Python does exit with a fatal error. Before, PyDict_GetItemString() failure was not handled: structseq_new() could call PyObject_GC_NewVar() with a negative size, and structseq_dealloc() could also crash. --- So moving from PyDict_GetItemString() to _PyDict_GetItemId() is for perfomances, but the main motivation is to handle better errors. I hope that the identifier will be initialized quickly at startup, and if its initialization failed, the failure is handled better... There is also a _PyDict_GetItemIdWithError() function. But it is not used currently (it was in changeset 2dd046be2c88). |
|||
msg202295 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-06 23:02 | |
New changeset 40c73ccaee95 by Victor Stinner in branch 'default': Issue #19512: __build_class() builtin now uses an identifier for the "metaclass" string http://hg.python.org/cpython/rev/40c73ccaee95 New changeset 7177363d8c5c by Victor Stinner in branch 'default': Issue #19512: fileio_init() reuses PyId_name identifier instead of "name" http://hg.python.org/cpython/rev/7177363d8c5c New changeset dbee50619259 by Victor Stinner in branch 'default': Issue #19512: _count_elements() of _collections reuses PyId_get identifier http://hg.python.org/cpython/rev/dbee50619259 New changeset 6a1ce1fd1fc0 by Victor Stinner in branch 'default': Issue #19512: builtin print() function uses an identifier instead of literal http://hg.python.org/cpython/rev/6a1ce1fd1fc0 |
|||
msg202296 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-06 23:03 | |
I changed the issue title to make it closer to the real changesets related to the issue. |
|||
msg202297 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-07 00:12 | |
New changeset 77bebcf5c4cf by Victor Stinner in branch 'default': Issue #19512: add _PyUnicode_CompareWithId() function http://hg.python.org/cpython/rev/77bebcf5c4cf New changeset 3f9f2cfae53b by Victor Stinner in branch 'default': Issue #19512: Use the new _PyId_builtins identifier http://hg.python.org/cpython/rev/3f9f2cfae53b |
|||
msg202320 - (view) | Author: Serhiy Storchaka (serhiy.storchaka) * | Date: 2013-11-07 09:19 | |
> What is the problem with these changes? Usually CPython team avoids code churn without serious reasons. Performance reasons for the change PySys_GetObject("stdout") to _PySys_GetObjectId(&_PyId_stdout) are ridiculous. You changed hundreds lines of code for speed up interactive mode by perhaps several microseconds. > Errors become more unlikely because objects are only initialized once, near startup. So it put also less pressure on code handling errors :) (it is usually the least tested part of the code) If there are bugs in code handling errors, they should be fixed in maintenance releases too. > You mean for PyRun_InteractiveOneObject()? Oh, it can be made private, but what is the problem of adding yet another PyRun_Interactive*() function? There are already a lot of them :-) And this is a problem. Newly added function is not even documented. > I also worked hard to support unencodable filenames: using char*, you cannot support arbitrary Unicode filename on Windows. That's why a added many various functions with "Object" suffix. Some examples: PyWarn_ExplicitObject(), PyParser_ParseStringObject(), PyImport_AddModuleObject(), etc. "One bug per bug report" as Martin says. > Another problem is that PyUnicode_FromString() failure is not handled correctly in some cases. PyUnicode_FromString() can fail because an decoder error, but also because of a MemoryError. It can't fail on "stdout" because an decoder error. |
|||
msg202324 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-07 10:35 | |
>> Another problem is that PyUnicode_FromString() failure is not handled correctly in some cases. PyUnicode_FromString() can fail because an decoder error, but also because of a MemoryError. > It can't fail on "stdout" because an decoder error. It can fail on "stdout" because of a memory allocation failure. |
|||
msg202325 - (view) | Author: Georg Brandl (georg.brandl) * | Date: 2013-11-07 11:39 | |
>> You mean for PyRun_InteractiveOneObject()? Oh, it can be made private, but what is the problem of adding yet another PyRun_Interactive*() function? There are already a lot of them :-) > And this is a problem. Newly added function is not even documented. Serhiy is right. You have to be responsible with the Py* namespace, and keep new functions private unless they are useful enough to the outside and you document them. In general, you changed lots of code without a single review. Can you slow down a bit? |
|||
msg202336 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-07 13:03 | |
> Serhiy is right. You have to be responsible with the Py* namespace, and keep new functions private unless they are useful enough to the outside and you document them. I created the issue #19518 to discuss this part (but also to propose other enhancements related to Unicode). |
|||
msg202337 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-07 13:10 | |
>> Errors become more unlikely because objects are only initialized once, near startup. So it put also less pressure on code handling errors :) (it is usually the least tested part of the code) > If there are bugs in code handling errors, they should be fixed in maintenance releases too. Well, using identifiers doesn't solve directly all issues. For example, _PyDict_GetItemId() should be replaced with _PyDict_GetItemIdWithError() to be complelty safe. It just reduces the probability of bugs. Using identifiers might add regressions for a minor gain (handling MemoryError better). As I did for issues #18048 and #19437 (related to issues found by failmalloc), I prefer to not backport such minor bugfixes to not risk a regression. > You changed hundreds lines of code for speed up interactive mode by perhaps several microseconds. Again, performance is not the main motivation, please read again msg202293. Or maybe you disagree with this message? Sorry, I didn't explain my changes in first messages of this issue. I created the issue to group my changesets to an issue, to explain why I did them. I didn't expect any discussion :-) But thank you for all your remarks. |
|||
msg202390 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-07 22:08 | |
New changeset 01c4a0af73cf by Victor Stinner in branch 'default': Issue #19512, #19515: remove shared identifiers, move identifiers where they http://hg.python.org/cpython/rev/01c4a0af73cf |
|||
msg202417 - (view) | Author: Roundup Robot (python-dev) | Date: 2013-11-08 13:07 | |
New changeset bf9c77bac36d by Victor Stinner in branch 'default': Issue #19512, #19526: Exclude the new _PyDict_DelItemId() function from the http://hg.python.org/cpython/rev/bf9c77bac36d |
|||
msg202698 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-12 15:43 | |
@Georg, Serhiy, Martin: Sorry for having commits directly without more review. I didn't expect negative feedback on such changes, I thaught to moving from literal C byte string to Python identifiers was a well accepted practice since identifiers are used in a lot of places in Python code base. So what should I do now? Should I revert all changesets related to this issue, or can we keep these new identifiers and close the issue? |
|||
msg203448 - (view) | Author: STINNER Victor (vstinner) * | Date: 2013-11-19 23:58 | |
No reaction, I close the issue. Reopen it if you still have complains ;-) |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:57:53 | admin | set | github: 63711 |
2013-11-19 23:58:52 | vstinner | set | status: open -> closed resolution: fixed messages: + msg203448 |
2013-11-12 15:43:28 | vstinner | set | nosy:
+ loewis messages: + msg202698 |
2013-11-08 13:07:48 | python-dev | set | messages: + msg202417 |
2013-11-07 22:08:08 | python-dev | set | messages: + msg202390 |
2013-11-07 16:48:16 | Arfrever | set | nosy:
+ Arfrever |
2013-11-07 13:10:30 | vstinner | set | messages: + msg202337 |
2013-11-07 13:03:50 | vstinner | set | messages: + msg202336 |
2013-11-07 11:39:47 | georg.brandl | set | nosy:
+ georg.brandl messages: + msg202325 |
2013-11-07 10:35:40 | vstinner | set | messages: + msg202324 |
2013-11-07 09:19:06 | serhiy.storchaka | set | messages: + msg202320 |
2013-11-07 00:12:37 | python-dev | set | messages: + msg202297 |
2013-11-06 23:03:48 | vstinner | set | messages:
+ msg202296 title: Avoid most calls to PyUnicode_DecodeUTF8Stateful() in Python interactive mode -> Avoid temporary Unicode strings, use identifiers to only create the string once |
2013-11-06 23:02:59 | python-dev | set | messages: + msg202295 |
2013-11-06 22:43:12 | vstinner | set | messages: + msg202293 |
2013-11-06 21:46:26 | python-dev | set | messages: + msg202291 |
2013-11-06 21:10:23 | vstinner | set | messages: + msg202288 |
2013-11-06 18:51:59 | serhiy.storchaka | set | messages: + msg202283 |
2013-11-06 18:06:22 | python-dev | set | messages: + msg202281 |
2013-11-06 17:47:32 | vstinner | set | messages: + msg202280 |
2013-11-06 17:45:51 | python-dev | set | nosy:
+ python-dev messages: + msg202279 |
2013-11-06 17:42:37 | vstinner | set | messages: + msg202278 |
2013-11-06 17:29:43 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages: + msg202277 |
2013-11-06 17:08:33 | vstinner | set | files:
+ pysys_getobjectid.patch keywords: + patch messages: + msg202275 |
2013-11-06 17:05:05 | vstinner | create |