[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[STINNER Victor]

STINNER Victor  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[STINNER Victor]

Change by STINNER Victor :


--
pull_requests: +22108
pull_request: https://github.com/python/cpython/pull/23209

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Thank you Raymond and Victor.

--
resolution:  -> fixed
stage:  -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[STINNER Victor]

STINNER Victor added the comment:

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

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--
nosy: +gvanrossum

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


Added file: http://bugs.python.org/file44115/py_setref_extra.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--
Added file: http://bugs.python.org/file42428/py_setref_extra.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

Thank you.  Can this be closed now?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--
keywords: +patch
Added file: http://bugs.python.org/file42376/py_setref.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Alexander Belopolsky]

Alexander Belopolsky added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Alexander Belopolsky]

Changes by Alexander Belopolsky :


--
nosy: +belopolsky

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[STINNER Victor]

Changes by STINNER Victor :


--
nosy:  -haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Stefan Krah]

Stefan Krah added the comment:

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

--
nosy: +skrah

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[STINNER Victor]

STINNER Victor added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[STINNER Victor]

STINNER Victor added the comment:

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

--
nosy: +haypo

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

Raymond Hettinger added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26200] SETREF adds unnecessary work in some cases

[Raymond Hettinger]

New submission from Raymond Hettinger:

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.

--
assignee: serhiy.storchaka
components: Interpreter Core
messages: 258913
nosy: rhettinger, serhiy.storchaka
priority: normal
severity: normal
status: open
title: SETREF adds unnecessary work in some cases
type: performance
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com