gh-141510, PEP 814: Add built-in frozendict type#144757
gh-141510, PEP 814: Add built-in frozendict type#144757vstinner wants to merge 10 commits intopython:mainfrom
Conversation
Add TYPE_FROZENDICT to the marshal module. Add C API functions: * PyAnyDict_Check() * PyAnyDict_CheckExact() * PyFrozenDict_Check() * PyFrozenDict_CheckExact() * PyFrozenDict_New() Add PyFrozenDict_Type C type.
vstinner
requested review from
AA-Turner,
ZeroIntensity,
ericsnowcurrently,
markshannon,
methane and
rhettinger
as code owners
|
cc @corona10 |
Fix also indentation.
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
| } | ||
|
|
||
| if (PyDict_CheckExact(d)) { | ||
| if (PyAnyDict_CheckExact(d)) { |
There was a problem hiding this comment.
Maybe same opinion with @kumaraditya303 we can avoid a lot of critical section if dict is frozen dict.
There was a problem hiding this comment.
I agree, but I would prefer to make such "optimization" change in a separated PR.
| dict_length(PyObject *self) | ||
| { | ||
| return FT_ATOMIC_LOAD_SSIZE_RELAXED(((PyDictObject *)self)->ma_used); | ||
| return GET_USED(_PyAnyDict_CAST(self)); |
There was a problem hiding this comment.
Do we have to use atomic operation for frozendict?
There was a problem hiding this comment.
No, we can skip the atomic operation for frozendict.
There was a problem hiding this comment.
It uses relaxed atomic so it compiles to simple loads so avoiding it has no real benefit.
| }; | ||
|
|
||
| static PyMappingMethods frozendict_as_mapping = { | ||
| .mp_length = dict_length, |
There was a problem hiding this comment.
I think that we can make frozendict_lenth
There was a problem hiding this comment.
I propose to make such optimization in a separated PR.
|
|
||
| static PyMappingMethods frozendict_as_mapping = { | ||
| .mp_length = dict_length, | ||
| .mp_subscript = dict_subscript, |
There was a problem hiding this comment.
Still perfer to make frozendict_subscript, but let's do it at the separate PR.
corona10
left a comment
corona10
left a comment
There was a problem hiding this comment.
We should notice that frozendict is not subclass of dict to follow SC feedback
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Document frozendict after lazy import.
| mp->ma_values == NULL && | ||
| (mp->ma_used >= (mp->ma_keys->dk_nentries * 2) / 3)) | ||
| (mp->ma_used >= (mp->ma_keys->dk_nentries * 2) / 3) && | ||
| !frozendict) |
There was a problem hiding this comment.
We might take this fast-path for frozendict as well. It's just a missed optimization opportunity.
@corona10: Add it to the optimization TODO list :-)
| if (items == NULL) { | ||
| return -1; | ||
| } | ||
| PyObject *frozenset = PyFrozenSet_New(items); |
There was a problem hiding this comment.
The current implementation creates an actual frozenset object which emits surprising error messages:
>>> hash(frozendict(x=[]))
Traceback (most recent call last):
File "<python-input-0>", line 1, in <module>
hash(frozendict(x=[]))
~~~~^^^^^^^^^^^^^^^^^^
TypeError: cannot use 'tuple' as a set element (unhashable type: 'list')
We may change the implementation later to not create an actual frozenset, but reuse frozenset hash code instead, to avoid the surprising errors.
Ah right, done. |
4 participants
Add TYPE_FROZENDICT to the marshal module.
Add C API functions:
Add PyFrozenDict_Type C type.
📚 Documentation preview 📚: https://cpython-previews--144757.org.readthedocs.build/