[ovs-dev] [PATCH ovn 17/21] ovn-northd-ddlog: Intern selected input relations.

[Ben Pfaff]

On Tue, Mar 30, 2021 at 05:52:05PM +0200, Dumitru Ceara wrote:
> On 3/27/21 1:31 AM, Ben Pfaff wrote:
> > From: Leonid Ryzhyk <lryzhyk at vmware.com>
> > 
> > DDlog 0.38.0 adds the `--intern-table` CLI flag to the `ovsdb2ddlog`
> > compiler to declare input tables coming from OVSDB as `Intern<...>`.
> > This is useful for tables whose records are copied around as a whole and
> > can therefore benefit from interning performance- and memory-wise.  In
> > the past we had to create separate tables in `helpers.dl` and copy
> > records from the original input table to them while wrapping them in
> > `Intern<>`.  With this change, we avoid the extra copy and intern
> > records as we ingest them for selected tables.
> > 
> > We use the `--intern-table` flag to eliminate all intermediate tables in
> > `helpers.dl`.
> > 
> > Signed-off-by: Leonid Ryzhyk <lryzhyk at vmware.com>
> > Signed-off-by: Ben Pfaff <blp at ovn.org>
> > ---
> 
> Hi Ben, Leonid,
> 
> This is not a full review, I just noticed that build fails with this
> patch applied.
> 
> > diff --git a/northd/ovsdb2ddlog2c b/northd/ovsdb2ddlog2c
> > index c66ad81073e1..19aeb265b633 100755
> > --- a/northd/ovsdb2ddlog2c
> > +++ b/northd/ovsdb2ddlog2c
> > @@ -36,6 +36,7 @@ The following ovsdb2ddlog options are supported:
> >    --output-only-table=TABLE  Mark TABLE as output-only.  DDlog will send updates to this table directly to OVSDB without comparing it with current OVSDB state.
> >    --ro=TABLE.COLUMN          Ignored.
> >    --rw=TABLE.COLUMN          Ignored.
> > +  --intern-table=TABLE       Ignored.
> >    --output-file=FILE.inc     Write output to FILE.inc. If this option is not specified, output will be written to stdout.
> >  
> >  The following options are also available:
> > @@ -52,6 +53,7 @@ if __name__ == "__main__":
> >                                                 'schema-file=',
> >                                                 'output-table=',
> >                                                 'output-only-table=',
> > +                                               'intern-table=',
> >                                                 'ro=',
> >                                                 'rw=',
> >                                                 'output-file='])
> > @@ -75,9 +77,7 @@ if __name__ == "__main__":
> >                  schema_file = value
> >              elif key in ['-o', '--output-table']:
> >                  output_tables.add(value)
> > -            elif key == '--output-only-table':
> > -                output_only_tables.add(value)
> 
> I guess the two lines above were removed by accident because now
> ovsdb2ddlog2c will silently exit when it receives '--output-only-table'
> as argument.

Hmm, odd.

I think that the correct fix here is to not remove those lines.  I'll
put that into the next version.

> As a side note, maybe it's good to improve the error reporting too so it
> doesn't silently exit when arguments are not known :)
> 
> > -            elif key in ['--ro', '--rw']:
> > +            elif key in ['--ro', '--rw', '--intern-table']:
> >                  pass
> >              elif key == '--output-file':
> >                  output_file = value

Yeah, good point, I'll send out a patch for that.