[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Łukasz Langa]

Change by Łukasz Langa :


--
resolution:  -> fixed
stage: patch review -> 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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Eric Lippert]

Eric Lippert  added the comment:

If it were possible that the interpreter iterating over a dictionary could 
cause the dictionary to change size then I suspect that this would be a rich 
source of bugs to mine. :-) 

It would be great if we could make it a documented, enforced invariant that the 
interpreter reading a dictionary never produces a side effect visible to the 
interpreter.

Re: "this specific statement already existed before my work."

Thanks for the historical note; the first thing I thought when I read this line 
was "that's got to be a copy-paste left over from an earlier version of the 
algorithm."

--

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Raymond Hettinger]

Change by Raymond Hettinger :


--
nosy: +tim.peters

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[STINNER Victor]

STINNER Victor  added the comment:

> I think it is technically not possible. Neither PyDict_Next() nor Py_INCREF() 
> mutate the dict, call the user code or release GIL. If it could be possible, 
> we would have a potential writing out of a buffer here.

When I read PyDict_Next(), I'm thinking at the Python level which can execute 
arbitrary code. But it seems like you are right: the exact C implementation of 
PyDict_Next() doesn't seem to modify the dictionary.

PR 9009 seems safe.

--

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

I think it is technically not possible. Neither PyDict_Next() nor Py_INCREF() 
mutate the dict, call the user code or release GIL. If it could be possible, we 
would have a potential writing out of a buffer here.

--

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[STINNER Victor]

STINNER Victor  added the comment:

Technically it's possible that the dictionary mutates itself during iterating 
and so that it's size change, so "nk = i / 2;" make sure that we pass the 
proper length to the function call.

Maybe I'm wrong, but we would need an unit test for that.

While I reworked a lot of code in call.c (and I moved code to call.c ;-)), this 
specific statement already existed before my work. I moved it from somewhere 
else. Extract of Objects/funcobject.c:

static PyObject *
function_call(PyObject *func, PyObject *arg, PyObject *kw)
{
...
if (kw != NULL && PyDict_Check(kw)) {
...
while (PyDict_Next(kw, , [i], [i+1])) {
Py_INCREF(k[i]);
Py_INCREF(k[i+1]);
i += 2;
}
nk = i/2;
}
...
}

It seems like the code is quite old :-)

commit 6d6c1a35e08b95a83dbe47dbd9e6474daff00354
Author: Tim Peters 
Date:   Thu Aug 2 04:15:00 2001 +

Merge of descr-branch back into trunk.

--

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Raymond Hettinger]

Change by Raymond Hettinger :


--
assignee:  -> vstinner
nosy: +serhiy.storchaka, vstinner

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Eric Lippert]

Change by Eric Lippert :


--
keywords: +patch
pull_requests: +8479
stage:  -> patch review

___
Python tracker 

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

[issue34551] Redundant store can be removed from _PyFunction_FastCallDict

[Eric Lippert]

New submission from Eric Lippert :

In _PyFunction_FastCallDict we have local nk assigned to be the size of a 
dictionary, and then local i is assigned to twice the size of the same 
dictionary, and then nk is assigned to half of i, which it already is:

   nk = (kwargs != NULL) ? PyDict_GET_SIZE(kwargs) : 0;
   if (nk != 0) {
 ...
 pos = i = 0;
 while (PyDict_Next(kwargs, , [i], [i+1])) {
   ...
   i += 2;
 }
 nk = i / 2;

I am attempting to understand the performance characteristics of this hot path, 
and I spent far too long trying to figure out why nk was being assigned a value 
it already has. :)

I propose that the redundant store be replaced with an assertion that i/2 is 
equal to nk.

I will submit a pull request presently.

--
components: Interpreter Core
messages: 324395
nosy: Eric Lippert
priority: normal
severity: normal
status: open
title: Redundant store can be removed from _PyFunction_FastCallDict
type: enhancement
versions: Python 3.8

___
Python tracker 

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