Repository navigation
Conversation
Use the new API in: * Modules/_abc.c * Modules/_asynciomodule.c * Modules/_hashopenssl.c * Modules/_ssl.c
Documentation build overview
44 files changed ·
|
vstinner
changed the title
PyFrozenSetWriter C APIPyFrozenSetWriter C API
|
Updating |
|
cc @corona10 |
|
This C API is just an abstraction on top of existing |
|
The edits to |
There multiple advantages of this new API. The current The internal frozenset is not tracked by the GC until Finish() is called (when the frozenset is fully initialized). So it's not possible to observe a frozenset being modified in Python (which must not be possible, frozenset are immutable, right?). See for example #159730 for recent examples of fixed bugs. Also, I think that's a bad design to have a PySet_Add() function to modify an immutable (frozenset) object. We should avoid such API. So I propose deprecating calling PySet_Add() on a frozenset (replaced with this new Writer API). See issues: |
|
When you're done making the requested changes, leave the comment: |
Add PyFrozenSetWriter_UpdateArray() and PyFrozenSetWriter_UpdateArraySteal() functions. Add tests on PyFrozenSetWriter_AddSteal(). Typoe: rename frozsetwriter_check_consistency() to frozensetwriter_check_consistency().
|
Updates:
|
|
|
||
|
|
||
| void | ||
| PyFrozenSetWriter_Discard(PyFrozenSetWriter *writer) |
There was a problem hiding this comment.
Can you name this something else? Sets already have a discard method with a completely different meaning. No need to create confusion by giving this the same name.
There was a problem hiding this comment.
Oh, it's unfortunate that PyFrozenSetWriter_Discard() name is close set.discard(), but has a different meaning (discard the temporary frozenset and destroy the writer) :-(
If possible, I would prefer to reuse Create/Finish/Discard names, since they are already used by 3 other "Writer" C API:
For now, I only propose functions to add items to the writer. If someone needs "more complex" use cases, I would suggest creating a mutable set, and then convert it to a frozenset (call PyFrozenNew_New(temp_set)).
If later a discard method is added, it can be called PyFrozenSetWriter_DiscardItem(writer, item).
Some cleanups.
|
UPDATE: I re-ran the benchmark on the latest PR (Finish now untracks properly the frozenset from the GC) using CPU isolation, I ran a benchmark on Linux (
writer_update calls the same code than frozenset_new, it just adds the PyFrozenSetWriter abstraction. Since the writer_update performance is basically the same as frozenset_new, it means that the abstraction is basically free in terms of performance. The 3 writer benchmarks use frozenset_new, writer_update and writer_update_array resizes the frozenset table at once, whereas writer_add has to resize the table multiple time (less efficient). frozenset_new and writer_update are tested with a tuple. They call PyObject_GetIter() and then loop on PyIter_Next(). frozenset_add and writer_update_array iterates on a C array of Python objects (no Python iterator protocol). size=0 and size=4 cases use the frozenset "small table": don't allocate a separated table on the heap. PyFrozenSetWriter_UpdateArray() is the fastest method to create a frozenset: 1.54x faster than Compare
Note: It might be interesting to optimize Note: I didn't test Free Threading since PySet_Add() and PyFrozenSetWriter avoid locking (critical section) anyway. Python benchmark script Detailsimport pyperf, functools, _testcapi
sizes = (0, 4, 100, 1_000, 100_000)
benchmarks = {
# Call PyFrozenSet_New() once:
# call PyObject_GetIter() and then loop on PyIter_Next()
'PyFrozenSet_New': _testcapi.bench_frozenset_new,
# Call PyFrozenSetWriter_Update() once:
# call PyObject_GetIter() and then loop on PyIter_Next()
'PyFrozenSetWriter_Update': _testcapi.bench_writer_update,
# Call PyFrozenSetWriter_Create(0) + PyFrozenSetWriter_Add() once per item
'PyFrozenSetWriter_Add': _testcapi.bench_writer_add,
# Call PyFrozenSetWriter_Create(0) + PyFrozenSetWriter_UpdateArray() once
'PyFrozenSetWriter_UpdateArray': _testcapi.bench_writer_update_array,
}
def add_cmdline_args(cmd, args):
cmd.append(args.bench)
runner = pyperf.Runner(add_cmdline_args=add_cmdline_args)
runner.argparser.add_argument('bench', choices=sorted(benchmarks))
args = runner.parse_args()
bench = args.bench
bench = benchmarks[bench]
for size in sizes:
test_tuple = tuple(range(size))
func = functools.partial(bench, test_tuple)
runner.bench_time_func(f'size={size:,}', func)Shell script Detailsset -e -x
export PYTHONHASHSEED=0
./python bench.py PyFrozenSet_New -o frozenset_new.json --inherit-environ=PYTHONHASHSEED
./python bench.py PyFrozenSetWriter_Update -o writer_update.json --inherit-environ=PYTHONHASHSEED
./python bench.py PyFrozenSetWriter_Add -o writer_add.json --inherit-environ=PYTHONHASHSEED
./python bench.py PyFrozenSetWriter_UpdateArray -o writer_update_array.json --inherit-environ=PYTHONHASHSEED
./python -m pyperf compare_to frozenset_new.json writer_update.json writer_add.json writer_update_array.jsonPatch adding _testcapi functions: Detailsdiff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
index 033ee3234cf..b2a881b4b4a 100644
--- a/Modules/_testcapimodule.c
+++ b/Modules/_testcapimodule.c
@@ -2889,6 +2889,139 @@ uptime_bsd(PyObject *Py_UNUSED(self), PyObject *Py_UNUSED(args))
#endif
+static PyObject*
+bench_frozenset_new(PyObject *Py_UNUSED(self), PyObject *args)
+{
+ PyObject *iterable;
+ Py_ssize_t loops;
+ if (!PyArg_ParseTuple(args, "On", &iterable, &loops)) {
+ return NULL;
+ }
+
+ PyTime_t t1, t2;
+ (void)PyTime_PerfCounterRaw(&t1);
+ for (Py_ssize_t i=0; i < loops; i++) {
+ PyObject *result = PyFrozenSet_New(iterable);
+ if (result == NULL) {
+ return NULL;
+ }
+ Py_DECREF(result);
+ }
+ (void)PyTime_PerfCounterRaw(&t2);
+
+ return PyFloat_FromDouble(PyTime_AsSecondsDouble(t2 - t1));
+}
+
+
+static PyObject*
+bench_writer_add(PyObject *Py_UNUSED(self), PyObject *args)
+{
+ PyObject *tuple;
+ Py_ssize_t loops;
+ if (!PyArg_ParseTuple(args, "On", &tuple, &loops)) {
+ return NULL;
+ }
+
+ Py_ssize_t len = PyTuple_GET_SIZE(tuple);
+
+ PyTime_t t1, t2;
+ (void)PyTime_PerfCounterRaw(&t1);
+ for (Py_ssize_t i=0; i < loops; i++) {
+ PyFrozenSetWriter *writer = PyFrozenSetWriter_Create(0);
+ if (writer == NULL) {
+ return NULL;
+ }
+
+ for (Py_ssize_t j=0; j < len; j++) {
+ PyObject *item = PyTuple_GET_ITEM(tuple, j);
+ if (PyFrozenSetWriter_Add(writer, item) < 0) {
+ PyFrozenSetWriter_Discard(writer);
+ return NULL;
+ }
+ }
+
+ PyObject *result = PyFrozenSetWriter_Finish(writer);
+ if (result == NULL) {
+ return NULL;
+ }
+ Py_DECREF(result);
+ }
+ (void)PyTime_PerfCounterRaw(&t2);
+
+ return PyFloat_FromDouble(PyTime_AsSecondsDouble(t2 - t1));
+}
+
+
+static PyObject*
+bench_writer_update(PyObject *Py_UNUSED(self), PyObject *args)
+{
+ PyObject *iterable;
+ Py_ssize_t loops;
+ if (!PyArg_ParseTuple(args, "On", &iterable, &loops)) {
+ return NULL;
+ }
+
+ PyTime_t t1, t2;
+ (void)PyTime_PerfCounterRaw(&t1);
+ for (Py_ssize_t i=0; i < loops; i++) {
+ PyFrozenSetWriter *writer = PyFrozenSetWriter_Create(0);
+ if (writer == NULL) {
+ return NULL;
+ }
+
+ if (PyFrozenSetWriter_Update(writer, iterable) < 0) {
+ PyFrozenSetWriter_Discard(writer);
+ return NULL;
+ }
+
+ PyObject *result = PyFrozenSetWriter_Finish(writer);
+ if (result == NULL) {
+ return NULL;
+ }
+ Py_DECREF(result);
+ }
+ (void)PyTime_PerfCounterRaw(&t2);
+
+ return PyFloat_FromDouble(PyTime_AsSecondsDouble(t2 - t1));
+}
+
+
+static PyObject*
+bench_writer_update_array(PyObject *Py_UNUSED(self), PyObject *args)
+{
+ PyObject *tuple;
+ Py_ssize_t loops;
+ if (!PyArg_ParseTuple(args, "On", &tuple, &loops)) {
+ return NULL;
+ }
+ PyObject **items = &PyTuple_GET_ITEM(tuple, 0);
+ Py_ssize_t len = PyTuple_GET_SIZE(tuple);
+
+ PyTime_t t1, t2;
+ (void)PyTime_PerfCounterRaw(&t1);
+ for (Py_ssize_t i=0; i < loops; i++) {
+ PyFrozenSetWriter *writer = PyFrozenSetWriter_Create(0);
+ if (writer == NULL) {
+ return NULL;
+ }
+
+ if (PyFrozenSetWriter_UpdateArray(writer, items, len) < 0) {
+ PyFrozenSetWriter_Discard(writer);
+ return NULL;
+ }
+
+ PyObject *result = PyFrozenSetWriter_Finish(writer);
+ if (result == NULL) {
+ return NULL;
+ }
+ Py_DECREF(result);
+ }
+ (void)PyTime_PerfCounterRaw(&t2);
+
+ return PyFloat_FromDouble(PyTime_AsSecondsDouble(t2 - t1));
+}
+
+
static PyMethodDef TestMethods[] = {
{"set_errno", set_errno, METH_VARARGS},
{"test_config", test_config, METH_NOARGS},
@@ -2988,6 +3121,10 @@ static PyMethodDef TestMethods[] = {
#ifdef HAVE_SYSCTLBYNAME
{"uptime_bsd", uptime_bsd, METH_NOARGS},
#endif
+ {"bench_frozenset_new", bench_frozenset_new, METH_VARARGS},
+ {"bench_writer_add", bench_writer_add, METH_VARARGS},
+ {"bench_writer_update", bench_writer_update, METH_VARARGS},
+ {"bench_writer_update_array", bench_writer_update_array, METH_VARARGS},
{NULL, NULL} /* sentinel */
};
|
I was wrong: PyFrozenSet_New(iterable) iterates on items to untrack the frozenset if an item is not tracked by the GC. |
|
I updated the PR.
|
|
I also ran benchmarks on "Steal" variants on Free Threading. PyFrozenSetWriter_AddSteal() compared to PyFrozenSetWriter_Add():
PyFrozenSetWriter_UpdateArraySteal() compared to PyFrozenSetWriter_UpdateArray():
Benchmark hidden because not significant (1): size=4 The difference is about -1 ns per item for PyFrozenSetWriter_AddSteal() and about -0.5 ns per item for PyFrozenSetWriter_UpdateArraySteal(). Hum, I'm not sure that it's really worth it to add these two "Steal" variant functions. |
Use the new API in:
PyFrozenWriterC API to create afrozensetobject #160147