This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Regression in memory use of shared key dictionaries for "compact dicts"
Type: behavior Stage: resolved
Components: Interpreter Core Versions:
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, lemburg, methane, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2020-03-30 15:13 by Mark.Shannon, last changed 2022-04-11 14:59 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
compact_dict_prevents_key_sharing.py Mark.Shannon, 2020-03-30 15:13
Pull Requests
URL Status Linked Edit
PR 28520 merged Mark.Shannon, 2021-09-22 15:05
PR 31550 merged methane, 2022-02-24 07:36
Messages (15)
msg365319 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-03-30 15:13
The current implementation of dicts prevents keys from being shared when the order of attribute differs from the first instance created.
This can potentially use a considerably larger amount of memory than expected.

Consider the class:

class C:

   opt = DEFAULT

   def __init__(self, attr, optional=None):
       if optional:
           self.opt = optional
       self.attr = attr

This is a reasonable way to write a class, but has unpredictable memory use.
In the attached example, per-instance dict size goes from 104 bytes to 232 bytes when sharing is prevented.

The language specification says that the dicts maintain insertion order, but the wording implies that this only to explicit dictionaries, not instance attribute or other namespace dicts.

Either we should allow key sharing in these cases, or clarify the documentation.
msg365320 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-30 15:23
But the following class should not lead to unlimited memory consumption when create new instances:

class C:
    count = 0
    def __init__(self):
        count = self.__class__.count
        self.__class__.count = count + 1
        setattr(self, f'a{count}', count)
msg365322 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-03-30 15:24
Indeed it shouldn't.
How is that relevant?
msg365325 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-03-30 15:39
Just to clarify.

class AlwaysShared:

   opt = DEFAULT

   def __init__(self, attr, optional=None):
       self.attr = attr
       if optional:
           self.opt = optional

class SometimesShared:

   opt = DEFAULT

   def __init__(self, attr, optional=None):
       if optional:
           self.opt = optional
       self.attr = attr

The class AlwaysShared always has shared keys, whereas the class SometimesShared is not shared if `optional` is True for first instantiation, but False for a later instantiation.
msg365326 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2020-03-30 15:39
I think the current behavior is a guard against such pitfall. If you allow to add new keys when not all other keys are set, you can end with sharing growing set of keys.
msg365390 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2020-03-31 15:25
You can't end up with a growing set of keys.
The problem is to do with the *order* of insertion, not the number of keys.
msg402443 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-09-22 15:02
This can be mitigated, if not entirely fixed, by storing an ordering bit vector in the values.

This way all instances of the class SometimesShared in the example above can share the keys.

The keys might be ("optional", "attr")

For any instances with only "attr" as an attibute, the values would be (NULL, value) and the order would be (1,)

The downsides of this approach are:
1. Each values, and thus dict needs an extra 64 bit value.
2. Shared keys have a maximum size of 16.

Overall, I expect the improved sharing to more than compensate for the disadvantages.
msg402465 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-09-22 19:02
> Overall, I expect the improved sharing to more than
> compensate for the disadvantages.

I expect the opposite.  This makes all dicts pay a price (in space, initialization time, and complexity) for a micro-optimization of an uncommon case (the normal case is for __init__ to run and set all the keys in a consistent order).  It is unlikely that the "benefits" to never be felt in real-word applications, but "disadvantages" would affect every Python program.

> The language specification says that the dicts maintain insertion 
> order, but the wording implies that this only to explicit 
> dictionaries, not instance attribute or other namespace dicts.

That is a quite liberal reading of the spec.  I would object to making instance and namespace dicts behave differently.  That would be a behavior regression and we would forever have to wrestle with the difference.
msg402468 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) Date: 2021-09-22 19:46
On 22.09.2021 21:02, Raymond Hettinger wrote:
>> The language specification says that the dicts maintain insertion 
>> order, but the wording implies that this only to explicit 
>> dictionaries, not instance attribute or other namespace dicts.
> 
> That is a quite liberal reading of the spec.  I would object to making instance and namespace dicts behave differently.  That would be a behavior regression and we would forever have to wrestle with the difference.

I agree. Keeping the insertion order is essential for many common
use cases, including those where a class or instance dict is used,
e.g. namespaces used for data records, data caches, field
definitions in data records, etc. (and yes, those often can be
dynamically extended as well :-)).

I think for the case you mention, a documentation patch would be
better and more helpful for the programmers. Point them to slots
and the sharing problem should go away in most cases :-)
msg402493 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-09-23 12:24
Raymond,

Only split dicts need the extra field.

Classes where many instances do not have exactly the same set of attributes may be more common than you think.
There are many reasons why some attributes may be added conditionally.

PR 28520 actually makes the dictionary code a bit simpler, as we don't need to maintain the invariant that value arrays cannot have holes. Maintaining an order is simple and cheap:

order = (order<<4) | insertion_index

There are pros and cons to both schemes: PR 28520 and the current implementation.

The problem with the current scheme is that it only works well for classes where all instances are initialized with exactly the same attributes, and in the same order.

The PR 28520 scheme can handle those cases where order and key set differ a bit, but has a maximum size of 16 before the dict must be combined.


We need as many instances as possible to have split dictionaries, to get https://github.com/faster-cpython/ideas/issues/72 working well as it will make the cost of not sharing even greater, relatively.
msg402494 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2021-09-23 12:50
There are several common idioms which do not work well with shared dictionaries.

1. Class attributes as defaults. If most instances of the class have the default value for some attribute, it can be set as the class attribute. It saves memory because most instances will have smaller dict. But if the first instance has such attribute, it cancels shared dict for all following instances without this attribute.

2. cached_property (and analogs). The cached instance attributes can be added in arbitrary order, canceling shared dict for this instance and maybe for all instances created later.

3. Some classes delete attributes in close() or __exit__() methods to avoid reference loops and to release resources earlier. Since shared dicts do not support deletion, such closed objects have now larger size than non-closed objects.
msg403299 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2021-10-06 12:20
New changeset a7252f88d3fa33036bdd6036b8c97bc785ed6f17 by Mark Shannon in branch 'main':
bpo-40116: Add insertion order bit-vector to dict values to allow dicts to share keys more freely. (GH-28520)
https://github.com/python/cpython/commit/a7252f88d3fa33036bdd6036b8c97bc785ed6f17
msg413884 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2022-02-24 06:47
I found regression caused by GH-28520.

```
class C:
    def __init__(self, n):
        if n:
            self.a = 1
            self.b = 2
            self.c = 3
        else:
            self.c = 1
            self.b = 2
            self.a = 3


o1 = C(True)
o2 = C(False)
print(o2.__dict__)  # {'c': 1, 'b': 2, 'a': 3}

d1 = {}
d1.update(o2.__dict__)  # {'a': 3, 'b': 2, 'c': 1}
print(d1)
```
msg413894 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2022-02-24 07:54
PyDict_Keys(), PyDict_Values(), and PyDict_Items() don't respect insertion order too.
msg414403 - (view) Author: Inada Naoki (methane) * (Python committer) Date: 2022-03-03 04:06
New changeset 4f74052b455a54ac736f38973693aeea2ec14116 by Inada Naoki in branch 'main':
bpo-40116: dict: Add regression test for iteration order. (GH-31550)
https://github.com/python/cpython/commit/4f74052b455a54ac736f38973693aeea2ec14116
History
Date User Action Args
2022-04-11 14:59:28adminsetgithub: 84297
2022-03-03 04:06:40methanesetmessages: + msg414403
2022-02-24 07:54:38methanesetmessages: + msg413894
2022-02-24 07:36:21methanesetpull_requests: + pull_request29671
2022-02-24 06:47:10methanesetmessages: + msg413884
2022-01-20 11:50:36Mark.Shannonsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2021-10-06 12:20:01Mark.Shannonsetmessages: + msg403299
2021-09-23 12:50:08serhiy.storchakasetmessages: + msg402494
2021-09-23 12:24:37Mark.Shannonsetmessages: + msg402493
2021-09-22 19:46:34lemburgsetnosy: + lemburg
messages: + msg402468
2021-09-22 19:02:33rhettingersetmessages: + msg402465
2021-09-22 15:05:03Mark.Shannonsetkeywords: + patch
stage: test needed -> patch review
pull_requests: + pull_request26911
2021-09-22 15:02:17Mark.Shannonsetmessages: + msg402443
2021-08-13 08:37:36Mark.Shannonsetnosy: + rhettinger
2020-03-31 15:25:08Mark.Shannonsetmessages: + msg365390
2020-03-30 15:39:34serhiy.storchakasetmessages: + msg365326
2020-03-30 15:39:15Mark.Shannonsetmessages: + msg365325
2020-03-30 15:24:34Mark.Shannonsetmessages: + msg365322
2020-03-30 15:23:07serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg365320
2020-03-30 15:13:56Mark.Shannoncreate