[issue26200] SETREF adds unnecessary work in some cases

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,

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Change by STINNER Victor : -- pull_requests: +22108 pull_request: https://github.com/python/cpython/pull/23209 ___ Python tracker ___

[issue26200] SETREF adds unnecessary work in some cases

Serhiy Storchaka added the comment: Thank you Raymond and Victor. -- resolution: -> fixed stage: -> resolved status: open -> closed ___ Python tracker

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Changes by Serhiy Storchaka : Added file: http://bugs.python.org/file44115/py_setref_extra.patch ___ Python tracker ___

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Serhiy Storchaka added the comment: Restored safe usage of Py_SETREFs introduced not in issue20440: in issue25928, changeset 3292b4862627, and issue25945. -- ___ Python tracker

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Raymond Hettinger added the comment: Thank you. Can this be closed now? -- ___ Python tracker ___ ___

[issue26200] SETREF adds unnecessary work in some cases

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:

[issue26200] SETREF adds unnecessary work in some cases

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:

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Serhiy Storchaka added the comment: Reraised this issue on Python-Dev: http://comments.gmane.org/gmane.comp.python.devel/156809 . -- ___ Python tracker

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Changes by Alexander Belopolsky : -- nosy: +belopolsky ___ Python tracker ___

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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.

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

Changes by STINNER Victor : -- nosy: -haypo ___ Python tracker ___ ___

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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

[issue26200] SETREF adds unnecessary work in some cases

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"

[issue26200] SETREF adds unnecessary work in some cases

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