Sorry, something went wrong.
|
Examples: >>> empty = frozendict()
>>> frozendict() is empty
True
>>> frozendict([]) is empty
True
>>> (empty | empty) is empty
True
>>> (empty | {}) is empty
True
>>> frozendict.fromkeys('') is empty
True
|
Sorry, something went wrong.
There was a problem hiding this comment.
Overall looks good to me, but give me enough time to take a look at see the detail.
Sorry, something went wrong.
|
I ran a microbenchmark on Linux with CPU isolation on this change:
I'm surprised by the slowdown on these operations:
bench.py script: Detailsimport pyperf
runner = pyperf.Runner()
# Get the singleton
runner.timeit('frozendict()',
stmt='frozendict()')
runner.timeit('frozendict([])',
setup='iterable = []',
stmt='frozendict(iterable)')
runner.timeit('frozendict((x for x in ()))',
stmt='frozendict((x for x in ()))')
runner.timeit('frozendict() | {}',
stmt='frozendict() | {}')
runner.timeit('frozendict.fromkeys("")',
stmt='frozendict.fromkeys("")')
# Operations on some small frozen dictionaries
runner.timeit('frozendict({"a": 1, "b": 2, "c": 3, "d": 4})',
setup='d = {"a": 1, "b": 2, "c": 3, "d": 4}',
stmt='frozendict(d)')
runner.timeit('frozendict(x=1, y=2) | frozendict(z=3)',
stmt='frozendict(x=1, y=2) | frozendict(z=3)')
runner.timeit('frozendict.fromkeys("abdef")',
stmt='frozendict.fromkeys("abdef")')
|
Sorry, something went wrong.
There was a problem hiding this comment.
I think it would make sense to also add this to Py_GetConstantBorrowed.
Sorry, something went wrong.
| PyMutex watcher_mutex; // Protects the watchers array (free-threaded builds) | ||
| _PyOnceFlag watcher_setup_once; // One-time optimizer watcher setup | ||
| PyDict_WatchCallback watchers[DICT_MAX_WATCHERS]; | ||
| PyObject *empty_frozendict; |
There was a problem hiding this comment.
Singletons are usually supposed to be stored on _PyRuntime.static_objects.singletons, not on the interpreter state. Is there a reason we're deviating from that convention here?
Sorry, something went wrong.
There was a problem hiding this comment.
Adding the empty frozendict singleton to _PyRuntime.static_objects.singletons would require to initialize a PyFrozenDictObject structure statically which is complicated. PyFrozenDictObject inherits from PyDictObject which has these two members:
We should get access to this empty_keys_struct (Py_EMPTY_KEYS) outside dictobject.c.
Pre-computing hash() is also complicated:
Well. Adding _PyDict_Init() and _PyDict_Fini() to allocate the empty frozendict singleton using dictobject.c code was simpler for me.
Sorry, something went wrong.
There was a problem hiding this comment.
Couldn't we do something like this?
You wouldn't have to get the details for empty_keys_struct outside of dictobject.c; I think you could just extern it.
Sorry, something went wrong.
|
I think it would make sense to also add this to Py_GetConstantBorrowed. Python had a frozenset singleton, but it was decided to remove it in Python 3.10. frozendict is still very new. I don't know if the empty frozendict singleton will stay forever. If we add Py_CONSTANT_EMPTY_FROZENDICT, we will have to keep the singleton forever, even if it will become less efficient in the future. So for now, I would prefer prefer to not add it to Py_GetConstant(). |
Sorry, something went wrong.
|
Python had a frozenset singleton, but it was decided to remove it in Python 3.10. Why was it removed? I'm fine with not adding one, but I'd like to know the context. |
Sorry, something went wrong.
|
Why was it removed? I'm fine with not adding one, but I'd like to know the context. #21085 removed the empty frozenset singleton in Python 3.10. |
Sorry, something went wrong.
|
Hmm, it seems it was removed because nobody was using it. Is there something that makes a frozendict different? (Or, in other words, do you have data showing that empty frozendict objects are common enough to warrant a singleton?) Sorry if I'm sounding grouchy -- I'm partially playing devil's advocate here. |
Sorry, something went wrong.
|
No, I didn't collect any stats about empty frozendict. |
Sorry, something went wrong.
|
Empty frozendicts: can be used anywhere we use MappingProxyType({}) as a default argument for a mapping parameter. People have module-wide constants say _EMPTY = ... and would use them instead of habing a None default. For frozenset() I am surprised but not that much because it is only used in default arguments when having a recursivenguard for instance. Such guard is rarer when writing C code though however a default frozendict() may be used (I still think it is more likely to use in pure Python rather than in C). |
Sorry, something went wrong.
|
I'm fine with this PR then. I'd appreciate investigating whether we can implement this on _PyRuntime instead of PyInterpreterState, but the approach in this PR works. |
Sorry, something went wrong.
The singleton is created immortal to avoid refcount contention in Free Threading.