Remove the tuple reuse optimization in _Pickle_FastCall.
I have noticed a race-condition occurring on one of the buildbots because of this optimization. The function called may release the GIL which means multiple threads may end up accessing the shared tuple. I could fix it up by storing the tuple to the Pickler and Unipickler object again, but honestly it really not worth the trouble. I ran many benchmarks and the only time the optimization helps is when using a fin-memory file, like io.BytesIO on which reads are super cheap, combined with pickle protocol less than 4. Even in this contrived case, the speedup is a about 5%. For everything else, this optimization does not provide any noticable improvements.
This commit is contained in:
parent
b6e66ebdf7
commit
b13e6bcbd8
@ -169,8 +169,6 @@ typedef struct {
|
|||||||
|
|
||||||
/* As the name says, an empty tuple. */
|
/* As the name says, an empty tuple. */
|
||||||
PyObject *empty_tuple;
|
PyObject *empty_tuple;
|
||||||
/* Single argument tuple used by _Pickle_FastCall */
|
|
||||||
PyObject *arg_tuple;
|
|
||||||
} PickleState;
|
} PickleState;
|
||||||
|
|
||||||
/* Forward declaration of the _pickle module definition. */
|
/* Forward declaration of the _pickle module definition. */
|
||||||
@ -208,7 +206,6 @@ _Pickle_ClearState(PickleState *st)
|
|||||||
Py_CLEAR(st->import_mapping_3to2);
|
Py_CLEAR(st->import_mapping_3to2);
|
||||||
Py_CLEAR(st->codecs_encode);
|
Py_CLEAR(st->codecs_encode);
|
||||||
Py_CLEAR(st->empty_tuple);
|
Py_CLEAR(st->empty_tuple);
|
||||||
Py_CLEAR(st->arg_tuple);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/* Initialize the given pickle module state. */
|
/* Initialize the given pickle module state. */
|
||||||
@ -328,8 +325,6 @@ _Pickle_InitState(PickleState *st)
|
|||||||
if (st->empty_tuple == NULL)
|
if (st->empty_tuple == NULL)
|
||||||
goto error;
|
goto error;
|
||||||
|
|
||||||
st->arg_tuple = NULL;
|
|
||||||
|
|
||||||
return 0;
|
return 0;
|
||||||
|
|
||||||
error:
|
error:
|
||||||
@ -342,40 +337,31 @@ _Pickle_InitState(PickleState *st)
|
|||||||
|
|
||||||
/* Helper for calling a function with a single argument quickly.
|
/* Helper for calling a function with a single argument quickly.
|
||||||
|
|
||||||
This has the performance advantage of reusing the argument tuple. This
|
|
||||||
provides a nice performance boost with older pickle protocols where many
|
|
||||||
unbuffered reads occurs.
|
|
||||||
|
|
||||||
This function steals the reference of the given argument. */
|
This function steals the reference of the given argument. */
|
||||||
static PyObject *
|
static PyObject *
|
||||||
_Pickle_FastCall(PyObject *func, PyObject *obj)
|
_Pickle_FastCall(PyObject *func, PyObject *obj)
|
||||||
{
|
{
|
||||||
PickleState *st = _Pickle_GetGlobalState();
|
|
||||||
PyObject *arg_tuple;
|
|
||||||
PyObject *result;
|
PyObject *result;
|
||||||
|
PyObject *arg_tuple = PyTuple_New(1);
|
||||||
|
|
||||||
|
/* Note: this function used to reuse the argument tuple. This used to give
|
||||||
|
a slight performance boost with older pickle implementations where many
|
||||||
|
unbuffered reads occurred (thus needing many function calls).
|
||||||
|
|
||||||
|
However, this optimization was removed because it was too complicated
|
||||||
|
to get right. It abused the C API for tuples to mutate them which led
|
||||||
|
to subtle reference counting and concurrency bugs. Furthermore, the
|
||||||
|
introduction of protocol 4 and the prefetching optimization via peek()
|
||||||
|
significantly reduced the number of function calls we do. Thus, the
|
||||||
|
benefits became marginal at best. */
|
||||||
|
|
||||||
arg_tuple = st->arg_tuple;
|
|
||||||
if (arg_tuple == NULL) {
|
if (arg_tuple == NULL) {
|
||||||
arg_tuple = PyTuple_New(1);
|
Py_DECREF(obj);
|
||||||
if (arg_tuple == NULL) {
|
return NULL;
|
||||||
Py_DECREF(obj);
|
|
||||||
return NULL;
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
assert(arg_tuple->ob_refcnt == 1);
|
|
||||||
assert(PyTuple_GET_ITEM(arg_tuple, 0) == NULL);
|
|
||||||
|
|
||||||
PyTuple_SET_ITEM(arg_tuple, 0, obj);
|
PyTuple_SET_ITEM(arg_tuple, 0, obj);
|
||||||
result = PyObject_Call(func, arg_tuple, NULL);
|
result = PyObject_Call(func, arg_tuple, NULL);
|
||||||
|
Py_CLEAR(arg_tuple);
|
||||||
Py_CLEAR(PyTuple_GET_ITEM(arg_tuple, 0));
|
|
||||||
if (arg_tuple->ob_refcnt > 1) {
|
|
||||||
/* The function called saved a reference to our argument tuple.
|
|
||||||
This means we cannot reuse it anymore. */
|
|
||||||
Py_CLEAR(arg_tuple);
|
|
||||||
}
|
|
||||||
st->arg_tuple = arg_tuple;
|
|
||||||
|
|
||||||
return result;
|
return result;
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -7427,7 +7413,6 @@ pickle_traverse(PyObject *m, visitproc visit, void *arg)
|
|||||||
Py_VISIT(st->import_mapping_3to2);
|
Py_VISIT(st->import_mapping_3to2);
|
||||||
Py_VISIT(st->codecs_encode);
|
Py_VISIT(st->codecs_encode);
|
||||||
Py_VISIT(st->empty_tuple);
|
Py_VISIT(st->empty_tuple);
|
||||||
Py_VISIT(st->arg_tuple);
|
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
Loading…
x
Reference in New Issue
Block a user