Skip to content

gh-125996: fix thread safety of ordered dict #133734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kumaraditya303
Copy link
Contributor

@kumaraditya303 kumaraditya303 commented May 9, 2025

Copy link
Contributor

@colesbury colesbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please also provide some timing measurements for basic operations?

register PyODictObject *od = _PyODictObject_CAST(op);
PyDict_Clear(op);
_odict_clear_nodes(od);
PyDict_Clear((PyObject *)self);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_PyDict_Clear_LockHeld

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_PyDict_Clear_LockHeld currently expects the critical section to held on the object but in tp_clear it is not held so it gives assertion failure, should I remove the assertion and use it instead?

The tp_clear of dict also currently uses PyDict_Clear and not _PyDict_Clear_LockHeld for likely similar reason I think.

cpython/Objects/dictobject.c

Lines 4647 to 4652 in 5de7e3f

static int
dict_tp_clear(PyObject *op)
{
PyDict_Clear(op);
return 0;
}

Comment on lines 1782 to 1773
#ifndef Py_GIL_DISABLED
Py_CLEAR(di->di_odict);
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we need to hold a lock for the iterator, we can keep the Py_CLEAR(di->di_odict);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed it now to first acquire the critical section on the iterator and also clears the dict when done, it still acquires the critical section on odict when looking up the next key.

@kumaraditya303
Copy link
Contributor Author

Would you please also provide some timing measurements for basic operations?

I added some benchmark for get/set operation to ftscaling and benchmarked it:

index 1a59e25189d..20f15200051 100644
--- a/Tools/ftscalingbench/ftscalingbench.py
+++ b/Tools/ftscalingbench/ftscalingbench.py
@@ -28,6 +28,7 @@
 import threading
 import time
 from operator import methodcaller
+from collections import OrderedDict
 
 # The iterations in individual benchmarks are scaled by this factor.
 WORK_SCALE = 100
@@ -43,6 +44,19 @@ def register_benchmark(func):
     ALL_BENCHMARKS[func.__name__] = func
     return func
 
+@register_benchmark
+def odict_get():
+    tab = OrderedDict((i, i) for i in range(100))
+    for i in range(1000 * WORK_SCALE):
+        tab.get(i % 100)
+
+@register_benchmark
+def odict_set():
+    tab = OrderedDict((i, i) for i in range(100))
+    for i in range(1000 * WORK_SCALE):
+        tab[i % 100] = i
+
+
 @register_benchmark
 def object_cfunction():
     accu = 0

Results:

./python.exe Tools/ftscalingbench/ftscalingbench.py odict_get odict_set
Running benchmarks with 10 threads
odict_get                  4.7x faster
odict_set                  4.4x faster

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants