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

[Terry Wilson]

Thanks for the review!

On Fri, Nov 5, 2021 at 5:43 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> 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:

Well that's embarrassing...

> 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"?

It should.

> > +    """
> > +    __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?

Not really. It was more just that I was replacing a dict and defaulted
to making it behave as dict-like as possible.

> > +
> > +    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.

True. I don't see any good reason to make this behave as
general-purpose as a normal dict. It'd be wrong to store non-column
data there.

> Thanks,
> Dumitru
>