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 <[email protected]> > --- > 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
