GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-141510, PEP 814: Add built-in frozendict type by vstinner · Pull Request #144757 · python/cpython Conversation
|
|
cc @corona10 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Fix also indentation.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Hugo van Kemenade <[email protected]>
There was an error while loading. Please reload this page.
| } | ||
|
|
||
| if (PyDict_CheckExact(d)) { | ||
| if (PyAnyDict_CheckExact(d)) { |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Maybe same opinion with @kumaraditya303 we can avoid a lot of critical section if dict is frozen dict.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I would prefer to make such "optimization" change in a separated PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| 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.
The reason will be displayed to describe this comment to others. Learn more.
Do we have to use atomic operation for frozendict?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
No, we can skip the atomic operation for frozendict.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
It uses relaxed atomic so it compiles to simple loads so avoiding it has no real benefit.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
len(frozendict) has been optimized by #144913.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| }; | ||
|
|
||
| static PyMappingMethods frozendict_as_mapping = { | ||
| .mp_length = dict_length, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I think that we can make frozendict_lenth
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I propose to make such optimization in a separated PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
|
||
| static PyMappingMethods frozendict_as_mapping = { | ||
| .mp_length = dict_length, | ||
| .mp_subscript = dict_subscript, |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Still perfer to make frozendict_subscript, but let's do it at the separate PR.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
We should notice that frozendict is not subclass of dict to follow SC feedback
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Co-authored-by: Hugo van Kemenade <[email protected]>
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.
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Yeah I will take a look at it
Sorry, something went wrong.
There was an error while loading. Please reload this page.
| if (items == NULL) { | ||
| return -1; | ||
| } | ||
| PyObject *frozenset = PyFrozenSet_New(items); |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I changed the implementation in #144919.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Ah right, done. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
🤖 New build scheduled with the buildbot fleet by @corona10 for commit 9101b1a 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F144757%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Let's wait build bot to pass the refleak test :) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I know this does not primarily belong to the PR, but it's such a small change that a new PR wouldn't make sense for these small "typos".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Adam Johnson <[email protected]>
Tests passed successfully on Refleaks buildbots: |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Hugo van Kemenade <[email protected]>
Add also a title in Doc/library/stdtypes.rst.
|
@hugovk: I modified a few more titles in the C API doc and the Python doc to use sentence case for headers. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
lgtm with current implementations , I will soon create PRs for removing unneeded overhead from my side.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Couple of nitpicks.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Hugo van Kemenade <[email protected]>
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Co-authored-by: Benedikt Johannes <[email protected]>
There was an error while loading. Please reload this page.
|
I merged my PR. Thank you very much for all your reviews! |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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. Co-authored-by: Hugo van Kemenade <[email protected]> Co-authored-by: Adam Johnson <[email protected]> Co-authored-by: Benedikt Johannes <[email protected]>
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. Co-authored-by: Hugo van Kemenade <[email protected]> Co-authored-by: Adam Johnson <[email protected]> Co-authored-by: Benedikt Johannes <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.