[ovs-dev] [PATCH] [python] Avoid pre-allocating column defaults

[Dumitru Ceara]

Hi Terry,

Nit: I'd prefix the commit subject line with "python: idl: ".

On 10/12/21 9:53 PM, Terry Wilson wrote:
> Many python implementations pre-allocate space for multiple
> objects in empty dicts and lists. Using a custom dict-like object
> that only generates these objects when they are accessed can save
> memory.
> 
> On a fairly pathological case where the DB has 1000 networks each
> with 100 ports, with only 'name' fields set, this saves around
> 300MB of memory.
> 
> One could argue that if values are not going to change from their
> defaults, then users should not be monitoring those columns, but
> it's also probably good to not waste memory even if user code is
> sub-optimal.

I agree, seems important to have this change.

I'm definitely no Python expert but the change looks OK to me; I just
have a couple of minor comments/questions below.

> 
> Signed-off-by: Terry Wilson <twilson at redhat.com>
> ---
>  python/ovs/db/custom_index.py |  1 -
>  python/ovs/db/idl.py          | 39 +++++++++++++++++++++++++++++------
>  2 files changed, 33 insertions(+), 7 deletions(-)
> 
> diff --git a/python/ovs/db/custom_index.py b/python/ovs/db/custom_index.py
> index 587caf5e3..cf6c0b8e1 100644
> --- a/python/ovs/db/custom_index.py
> +++ b/python/ovs/db/custom_index.py
> @@ -18,7 +18,6 @@ OVSDB_INDEX_DESC = "DESC"
>  ColumnIndex = collections.namedtuple('ColumnIndex',
>                                       ['column', 'direction', 'key'])
>  
> -

This is unrelated and makes flake8 fail:

2021-11-05T10:23:25.4632044Z ../../python/ovs/db/custom_index.py:21:1:
E302 expected 2 blank lines, found 1
2021-11-05T10:23:25.5069538Z Makefile:5831: recipe for target
'flake8-check' failed

>  class MultiColumnIndex(object):
>      def __init__(self, name):
>          self.name = name
> diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
> index ecae5e143..de78bc79f 100644
> --- a/python/ovs/db/idl.py
> +++ b/python/ovs/db/idl.py
> @@ -45,6 +45,36 @@ Notice = collections.namedtuple('Notice', ('event', 'row', 'updates'))
>  Notice.__new__.__defaults__ = (None,)  # default updates=None
>  
>  
> +class ColumnDefaultDict(dict):
> +    """A column dictionary with on-demand generated default values
> +
> +    This object acts like the Row.__data__ column dictionary, but without the
> +    neccessity of populating columnn default values. These values are generated

Typos: "neccessity" and "columnn".

> +    on-demand and therefor only use memory once they are accessed.

Shouldn't this be "therefore"?

> +    """
> +    __slots__ = ('_table', )
> +
> +    def __init__(self, table, *args, **kwargs):
> +        self._table = table
> +        super().__init__(*args, **kwargs)

Do you foresee a use case for passing custom columns that are not part
of the table definition?

> +
> +    def __missing__(self, column):
> +        column = self._table.columns[column]
> +        return ovs.db.data.Datum.default(column.type)
> +
> +    def keys(self):
> +        return super().keys() | self._table.columns.keys()

Based on the answer to the question above we might not need this union.

Thanks,
Dumitru