[issue24681] Put most likely test first in set_add_entry()

Changes by Raymond Hettinger : -- resolution: -> fixed status: open -> closed ___ Python tracker ___ ___ Python-bugs-list mailing lis

[issue24681] Put most likely test first in set_add_entry()

Roundup Robot added the comment: New changeset 9e3be159d023 by Raymond Hettinger in branch 'default': Issue #24681: Move the most likely test first in set_add_entry(). https://hg.python.org/cpython/rev/9e3be159d023 -- ___ Python tracker

[issue24681] Put most likely test first in set_add_entry()

Changes by STINNER Victor : -- nosy: -haypo ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe: https://mail.pytho

[issue24681] Put most likely test first in set_add_entry()

Serhiy Storchaka added the comment: The patch makes set_add_entry() inconsistent not only with dict, but with other set methods that use set_lookkey(). I don't know if there is some subtle bug here, but I feel myself slightly uncomfortable with it. Also the new code looks a little less readabl

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: Serhiy, do you see anything actually wrong with the patch? If it isn't broken in some clear cut way, I'm going to apply it shortly. The comparison with dict internals is a red herring -- there is no promise need or precedent for making that code exactly t

[issue24681] Put most likely test first in set_add_entry()

Changes by Raymond Hettinger : Added file: http://bugs.python.org/file39993/move_likely_first.diff ___ Python tracker ___ ___ Python-bugs-list

[issue24681] Put most likely test first in set_add_entry()

Changes by Raymond Hettinger : Removed file: http://bugs.python.org/file39992/move_likely_first.diff ___ Python tracker ___ ___ Python-bugs-li

[issue24681] Put most likely test first in set_add_entry()

Changes by Raymond Hettinger : Added file: http://bugs.python.org/file39992/move_likely_first.diff ___ Python tracker ___ ___ Python-bugs-list

[issue24681] Put most likely test first in set_add_entry()

Changes by Raymond Hettinger : -- assignee: serhiy.storchaka -> rhettinger ___ Python tracker ___ ___ Python-bugs-list mailing list Un

[issue24681] Put most likely test first in set_add_entry()

Roundup Robot added the comment: New changeset bc80c783c4ab by Raymond Hettinger in branch 'default': Issue #24681: Move the store of so->table to the code block where it is used. https://hg.python.org/cpython/rev/bc80c783c4ab -- nosy: +python-dev __

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: Sets are under no obligation to keep their code synced with dicts and have over time diverged in a number of ways. -- ___ Python tracker ___

[issue24681] Put most likely test first in set_add_entry()

Serhiy Storchaka added the comment: The condition that is determined by the PyObject_RichCompareBool() check is not that the element is already present, but that the element was present. Even if we will agree that such behavior change is appropriate, the inconsistency with dict makes it doubtf

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: If PyObject_RichCompareBool() reports that a key is already present in the set, then set_add_entry() is under no further obligation to make an insertion. As long as there is no risk of segfault, there is no more obligation to cater to lying or self-destru

[issue24681] Put most likely test first in set_add_entry()

STINNER Victor added the comment: For me, optimizing assembler is still an optimization. I give up, I just don't care of set performances. -- ___ Python tracker ___

[issue24681] Put most likely test first in set_add_entry()

Eric Snow added the comment: Thanks for the clear explanation, Raymond. The approach you've described is useful in a number of circumstances. Would you mind publishing (somewhere outside the tracker; devguide?) the specific steps you take and the tools you use? -- nosy: +eric.snow

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: > "you have to provide a benchmark" Actually, I don't. When making a small series of changes, benchmarking every step is waste of time and tends to trap you in local minimums and causes you to overfit to a particular processor, compiler, or benchmark. The

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: FWIW, my approach is to look at the most important code paths to see if there is any work being done that isn't essential for the result being computed. Next, I look at the generated assembly to estimate speed by counting memory accesses (and whether they are

[issue24681] Put most likely test first in set_add_entry()

R. David Murray added the comment: Victor, I'm hearing Raymond say that it isn't really about optimization, but about the logical organization of the code. I think making things more *complicated* requires a benchmark justification, but it doesn't look to me like this change makes things more

[issue24681] Put most likely test first in set_add_entry()

Serhiy Storchaka added the comment: The patch changes behavior. With the patch it would be possible that after successful set.add(), the item will be not contained in the set. And this behavior is not consistent with dict behavior. -- ___ Python tra

[issue24681] Put most likely test first in set_add_entry()

STINNER Victor added the comment: > You can benchmark if you want. No, you have to provide a benchmark if you want to modify the Python core to optimize it. Otherwise, modifying the code is pointless. It's a general rule in Python to optimize code. We don't accept optimization changes if it h

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: You can benchmark if you want. I'm looking for a second pair of eyes to validate the correctness. My goal is to put the tests and assignments in the most logical order. -- ___ Python tracker

[issue24681] Put most likely test first in set_add_entry()

STINNER Victor added the comment: Since it looks like an optimization, can you please provide a benchmark? -- nosy: +haypo ___ Python tracker ___

[issue24681] Put most likely test first in set_add_entry()

Raymond Hettinger added the comment: One other change is to defer saving table=so->table until just before the rich comparison since it is only needed in that code block. This improves the generated code saving a register spill and reload on the most common code paths. -- Added file:

[issue24681] Put most likely test first in set_add_entry()

Changes by Raymond Hettinger : -- title: Put most likely test first is set_add_entry() -> Put most likely test first in set_add_entry() ___ Python tracker ___ __