SETREF adds unnecessary work in some cases

[rhettinger]

BPO	26200
Nosy	@gvanrossum, @rhettinger, @abalkin, @vstinner, @skrah, @serhiy-storchaka
PRs	[WIP] bpo-42294: Add Py_SetRef() and Py_XSetRef() #23209
Files	py_setref.patch py_setref_extra.patch py_setref_extra.patch

^{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']

[rhettinger]

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]

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]

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

[rhettinger]

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

[serhiy-storchaka]

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

[python-dev]

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]

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

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]

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]

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]

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]

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]

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