Repository navigation
SETREF adds unnecessary work in some cases #70388
Description
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 = 'https://github.com/serhiy-storchaka'
closed_at = <Date 2016-08-17.18:22:32.545>
created_at = <Date 2016-01-25.18:03:44.464>
labels = ['interpreter-core', 'performance']
title = 'SETREF adds unnecessary work in some cases'
updated_at = <Date 2020-11-09.22:09:20.062>
user = 'https://github.com/rhettinger'bugs.python.org fields:
activity = <Date 2020-11-09.22:09:20.062>
actor = 'serhiy.storchaka'
assignee = 'serhiy.storchaka'
closed = True
closed_date = <Date 2016-08-17.18:22:32.545>
closer = 'serhiy.storchaka'
components = ['Interpreter Core']
creation = <Date 2016-01-25.18:03:44.464>
creator = 'rhettinger'
dependencies = []
files = ['42376', '42428', '44115']
hgrepos = []
issue_num = 26200
keywords = ['patch']
message_count = 28.0
messages = ['258913', '259774', '259784', '259858', '259874', '259913', '260048', '260075', '260088', '260101', '260132', '260134', '262204', '262825', '262933', '262934', '262939', '263144', '263165', '263167', '263168', '263169', '272744', '272745', '272919', '272980', '380586', '380620']
nosy_count = 7.0
nosy_names = ['gvanrossum', 'rhettinger', 'belopolsky', 'vstinner', 'skrah', 'python-dev', 'serhiy.storchaka']
pr_nums = ['23209']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue26200'
versions = ['Python 3.6']
Activity
rhettinger commentedon Jan 25, 2016
Application of the SETREF macro was not code neutral in a number of cases. The SETREF macro always uses Py_XDECREF where the original code correctly used a faster and lighter Py_DECREF.
There should be an XSETREF variant and more care should be taken when applying these macros wholesale to the entire code base.
rhettinger commentedon Feb 7, 2016
If you don't add an XSETREF variant, I think you should revert the instances where a Py_DECREF was downgraded to Py_XDECREF.
serhiy-storchaka commentedon Feb 7, 2016
On 28.02.14 15:58, Kristján Valur Jónsson wrote:
On 16.12.15 16:53, Random832 wrote:
It was my initial intension. But then I had got a number of voices for single macros.
On 16.12.15 23:16, Victor Stinner wrote:
On 17.12.15 08:22, Nick Coghlan wrote:
Some objections were repeated by their authors few times. And I had no one voice for separate macros (except my).
In the light of your objection we should reraise this issue on Python-Dev.
Now, after applying patches, it would be harder to split Py_SETREF on two macros. But I tried to not replace a Py_DECREF with a Py_SETREF in performance critical code (e.g. in PyDict_SetItem).
rhettinger commentedon Feb 8, 2016
Sorry I wasn't in the conversation to back you up.
If you don't mind, I would like to restore the Py_DECREF in deque_ass_item().
serhiy-storchaka commentedon Feb 8, 2016
Oh, sorry, I didn't considered the deque as essential class. Please do. Py_SETREF here only saved 3 lines of code.
python-dev commentedon Feb 9, 2016
New changeset 3084914245d2 by Raymond Hettinger in branch 'default':
Issue bpo-26200: The SETREF macro adds unnecessary work in some cases.
https://hg.python.org/cpython/rev/3084914245d2
vstinner commentedon Feb 10, 2016
The code using Py_DECREF() doesn't look to be vulnerable to the bug described in Py_SETREF() doc. The change (revert) looks good to me.
So I understand that calling Py_DECREF() manually (in the "right" order, as done in the change) is a reasonable answer to users wanting a "fast" Py_SETREF().
What do you think?
I don't suggest to mention it in Py_SETREF() documentation. I would prefer that users who don't understand well reference counting (ex: me ;-)) use Py_SETREF() to avoid bugs.
rhettinger commentedon Feb 11, 2016
Most of the SETREFs in the next() functions in the itertools module should also be restored to there former state. They look to have been correct before the change, so there was no improvement, only degradation. Much of that code had been finely tuned and battle tested over many years -- the addition of SETREFs was gratuitous.
vstinner commentedon Feb 11, 2016
Raymond Hettinger: "Most of the SETREFs in the next() functions in the itertools module should also be restored to there former state. They look to have been correct before the change, so there was no improvement, only degradation. Much of that code had been finely tuned and battle tested over many years -- the addition of SETREFs was gratuitous."
I guess that your concern is performance. Did you notice a performance difference on a micro-benchmark? I don't think that it's possible to see any difference with the addition of a single if in C.
I disagree that the change is gratuitous: it was discussed at length and approved on python-dev and the issue bpo-20440. The idea was not new, it was already proposed in issue bpo-3081 (opened in 2008). The issue bpo-20440 (Py_SETREF) is a generic fix of the bug bpo-16447. PyPy dev found crazy bugs in CPython :-)
IMHO correctness matters more than performance here.
Oh, and by the way, I like a macro which avoids 3 lines of C code ;-) It makes the code shorter and more readable.
serhiy-storchaka commentedon Feb 11, 2016
Py_SETREF() is used not only for fixing possible bugs. It can make the correct code shorter and cleaner. For example Stephan appreciated this in df78978dacab. In most case the difference between Py_DECREF and Py_XDECREF looks negligible.
Most of the SETREFs in the next() functions in the itertools module replace Py_CLEAR and therefore don't add additional overhead (but may fix possible bugs). The only Py_DECREF were replaced in tee_next() and accumulate_next(). But they are used not in tight loop. Py_SETREF in tee_next() is executed only for every 57th call. It unlikely causes any measurable degradation. Py_SETREF in accumulate_next() is used together with calling PyIter_Next and PyNumber_Add() or PyObject_CallFunctionObjArgs() which takes much more time.
In any case it would be nice to see any measurements that show a degradation.
rhettinger commentedon Feb 11, 2016
It may just be me, but I find the code the be less readable. The presence of the SETREFs in the itertools modules makes it harder for me to count and track all the references to make sure the code is correct. For me, it is an obstacle to maintenance.
The itertools code was carefully thought-out, reviewed, clear (at least to its creator and maintainer), and finely tuned. It has been stable for a very long time and I don't think it should have been changed.
The module was designed for high-performance and I'm opposed to adding unnecessary work. As you know, these kind of things are very difficult to run timings on, but it is clear to both of us that the wholesale replacement of Py_DECREF with Py_XDECREF adds unnecessary load to the Branch Target Buffer and to the I-cache. In general, unnecessary work is always a step in the wrong direction, particularly in a module designed for performance.
Another occasional issue with the SETREF macro is that gets it the way of trying to defer all decrefs until as late as possible in a function call. The LRU cache is a example of a place where we want to bring the whole data structure into a coherent state prior to any of the decrefs (I even have to do that in the pure python lru cache code to guard against premature re-entrancy).
As the creator and principal maintainer of itertools, I'm stating a very strong preference to restore the code in the next() methods.
Please don't make this hard for me (When one of the most experienced of the active developers states a strong code preference and gives the reasons for it, there shouldn't have to be struggle to get it done).
skrah commentedon Feb 11, 2016
As Serhiy mentioned, I'm really happy with the Py_SETREF() macro and I understand the reasons why it was applied so broadly.
But if a module maintainer prefers not to change existing (and
correct) code, then that should have priority (also, the existing
version was quite readable).
3 remaining items
rhettinger commentedon Apr 6, 2016
Please apply the new macros so that all the original DECREFs are restored rather than blindly converted to XDECREFs.
serhiy-storchaka commentedon Apr 6, 2016
Py_SETREF was renamed to Py_XSETREF in 719c11b6b6ff, d0c8b2c1544e and 7197809a7428.
Here is a patch that introduces new Py_SETREF and uses it instead Py_XSETREF if Py_DECREF was used before.
python-dev commentedon Apr 10, 2016
New changeset 9cf8572abe58 by Serhiy Storchaka in branch '2.7':
Issue bpo-26200: Added Py_SETREF and replaced Py_XSETREF with Py_SETREF
https://hg.python.org/cpython/rev/9cf8572abe58
New changeset 66fafa13a711 by Serhiy Storchaka in branch '3.5':
Issue bpo-26200: Added Py_SETREF and replaced Py_XSETREF with Py_SETREF
https://hg.python.org/cpython/rev/66fafa13a711
New changeset d758c5965199 by Serhiy Storchaka in branch 'default':
Issue bpo-26200: Added Py_SETREF and replaced Py_XSETREF with Py_SETREF
https://hg.python.org/cpython/rev/d758c5965199
rhettinger commentedon Apr 11, 2016
Thank you. Can this be closed now?
python-dev commentedon Apr 11, 2016
New changeset aa5dbc32d313 by Serhiy Storchaka in branch '3.5':
Issue bpo-26200: Restored more safe usages of Py_SETREF.
https://hg.python.org/cpython/rev/aa5dbc32d313
New changeset f21740a1abde by Serhiy Storchaka in branch '2.7':
Issue bpo-26200: Restored more safe usages of Py_SETREF.
https://hg.python.org/cpython/rev/f21740a1abde
New changeset 144b78ac2077 by Serhiy Storchaka in branch 'default':
Issue bpo-26200: Restored more safe usages of Py_SETREF.
https://hg.python.org/cpython/rev/144b78ac2077
serhiy-storchaka commentedon Apr 11, 2016
Restored safe usage of Py_SETREFs introduced not in bpo-20440: in bpo-25928, changeset 3292b4862627, and bpo-25945.
serhiy-storchaka commentedon Apr 11, 2016
Here is a patch that adds extra usages of the Py_SETREF() macro (in particular in deque implementation). It doesn't fix bugs or improve performance, but makes the code shorter and perhaps makes it more readable.
If you think that some of these changes really improve readability (I think not all of them), let me know and I'll commit selected changes. Otherwise I'll just close this issue.
Currently Py_XSETREF is used 118 times and Py_SETREF is used 59 times. py_setref_extra.patch adds 44 new usages of Py_SETREF.
rhettinger commentedon Aug 15, 2016
I don't think most of these should be done. In most of these cases, the code is very old, stable, readable, and shouldn't be churned unnecessarily.
Also, putting a function call inside a macro is a worrisome practice in C. Ordinarily, we have long preferred a style of putting the components on separate lines to make the code less magical.
Ask Guido whether he thinks any of the above are a good idea? While it is a bit shorter, it also interferes with my ability to decrypt and reason about the code (especially when I'm trying to count references or trying to ascertain whether an error condition has been checked).
serhiy-storchaka commentedon Aug 15, 2016
Guido, what is your thought?
I sometimes use Py_SETREF() in new code for simplicity (and not only me). I agree that in case of long complex expression putting it inside a macro looks ugly.
vstinner commentedon Aug 17, 2016
I conccur with Raymond: it can be very painful if you get a segfault on such line. What is crashing? The function call? DECREF? INCREF? something else?
It's also more painful to debug such code in gdb step by step.
I dislike py_setref_extra.patch. I prefer more verbose C code, easy to debug.
serhiy-storchaka commentedon Aug 17, 2016
Thank you Raymond and Victor.
vstinner commentedon Nov 9, 2020
I wrote PR 23209 to add Py_SetRef() and Py_XSetRef() functions to the limited C API and the stable ABI. Py_SETREF() and Py_XSETREF() macros are excluded from the limited C API and some extension modules cannot use them, like extension modules written in Rust.
serhiy-storchaka commentedon Nov 9, 2020
Py_SETREF() is simple wrappers around Py_DECREF() and assignment. If there are macros in Rust it is better to implement Py_SETREF() as Rust macro.
Py_SETREF() was not added to the limited API for reason. If arguments against Py_SETREF() are still valid, the same arguments are applicable to Py_SetRef(). And even if Py_SETREF() will be added to the limited C API I do not think that there is a need of adding Py_SetRef(). It is more cumbersome (additional &), slower and affects the optimization of surrounded code (pointer cannot be in register), and does not have any advantage in addition to the macro.
This issue was closed 4 years ago. Please open a new issue for Py_SetRef().