Thanks for the review! On Fri, Nov 5, 2021 at 5:43 AM Dumitru Ceara <dce...@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 <twil...@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 > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev