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.

If you don't add an XSETREF variant, I think you should revert the instances where a Py_DECREF was downgraded to Py_XDECREF.

On 28.02.14 15:58, Kristján Valur Jónsson wrote:
> Also, for the equivalence to hold there is no separate Py_XSETREF, the X
> behaviour is implied, which I favour.  Enough of this X-proliferation
> already!

On 16.12.15 16:53, Random832 wrote:
> I think "SET" names imply that it's safe if the original
> reference is NULL. This isn't an objection to the names, but if
> it is given one of those names I think it should use Py_XDECREF.

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:
> I would prefer a single macro to avoid bugs, I don't think that such
> macro has a critical impact on performances. It's more designed for
> safety, no?

On 17.12.15 08:22, Nick Coghlan wrote:
>> 1. Py_SETREF
>
> +1 if it always uses Py_XDECREF on the previous value (as I'd expect
> this to work even if the previous value was NULL)

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).

> And I had no one voice for separate macros (except my).

Sorry I wasn't in the conversation to back you up.

> I tried to not replace a Py_DECREF with a Py_SETREF 
> in performance critical code (e.g. in PyDict_SetItem).

If you don't mind, I would like to restore the Py_DECREF in deque_ass_item().

Oh, sorry, I didn't considered the deque as essential class. Please do. Py_SETREF here only saved 3 lines of code.

New changeset 3084914245d2 by Raymond Hettinger in branch 'default':
Issue #26200:  The SETREF macro adds unnecessary work in some cases.
https://hg.python.org/cpython/rev/3084914245d2

> New changeset 3084914245d2 by Raymond Hettinger in branch 'default':
> Issue #26200:  The SETREF macro adds unnecessary work in some cases.
> https://hg.python.org/cpython/rev/3084914245d2

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.

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.

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 #20440. The idea was not new, it was already proposed in issue #3081 (opened in 2008). The issue #20440 (Py_SETREF) is a generic fix of the bug #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.

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.

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).

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).

I am late to this discussion, but FWIW, I would like to back Raymond up.  For me, Py_XDECREF is usually a sign of lazy programming and an optimization opportunity.  In many cases I've seen, Py_XDECREF is used under a "done:" label and can be optimized by strategically placing Py_DECREFs before (error) returns while keeping track of what is and what is not initialized. In addition to an extra null check, Py_XDECREF typically requires a NULL initialization which can be avoided with a more thoughtful code structure.

All in all, Py_XDECREF rightfully stands out with an "X" spelling.  I don't want to see it hidden behind an innocent-looking convenience macro.

I am ±0 on the XSETREF variant, but I think SETREF should use DECREF.

Reraised this issue on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/156809 .

Since several core devs agree that we should have doual macros, I'll rename Py_SETREF to Py_XSETREF and add new Py_SETREF.

Please apply the new macros so that all the original DECREFs are restored rather than blindly converted to XDECREFs.

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.

New changeset 9cf8572abe58 by Serhiy Storchaka in branch '2.7':
Issue #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 #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 #26200: Added Py_SETREF and replaced Py_XSETREF with Py_SETREF
https://hg.python.org/cpython/rev/d758c5965199

Thank you.  Can this be closed now?

New changeset aa5dbc32d313 by Serhiy Storchaka in branch '3.5':
Issue #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 #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 #26200: Restored more safe usages of Py_SETREF.
https://hg.python.org/cpython/rev/144b78ac2077

Restored safe usage of Py_SETREFs introduced not in issue20440: in issue25928, changeset 3292b4862627, and issue25945.

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.

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.

   Py_SETREF(result, PyNumber_Add(result, item));
   Py_SETREF(buffer, PyUnicode_AsEncodedString(buffer, "utf-8", "surrogatepass"));
   Py_SETREF(text, _PyObject_CallMethodId(text, &PyId_replace, "ss", "\n", self->writenl));

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).

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.

> Also, putting a function call inside a macro is a worrisome practice in C. 

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.

Thank you Raymond and Victor.

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.

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().