[ovs-dev] [PATCH ovn v3 00/27] ddlog 5x performance improvement

[Mark Michelson]

Hi Ben,

I gave these patches another look, and I can say that for patches 1-10, 
and patch 27:

Acked-by: Mark Michelson <mmichels at redhat.com>

For patch 11, I had suggested previously a configure-time check to 
detect if the correct DDLog version was installed, and you replied with 
an example that you would roll into the next version of the series. But 
I don't see it here. Is it in a different patch series?

For patches 12-26, I don't feel qualified to definitively state that 
it's the "best" way to optimize things, but the changes all make sense 
to me and don't seem to be incorrect, so for patches 12-26:

Acked-by: Mark Michelson <mmichelson at redhat.com>

but I wouldn't be averse to others having a look to be certain.

Thanks!

On 5/7/21 12:06 AM, Ben Pfaff wrote:
> This series of patches greatly improves the performance of
> ovn-northd-ddlog with the benchmark added in the final patch.  The
> first three patches improve both the benchmark for both versions of
> ovn-northd.
> 
> Here are the timings that I measure in each case.  All of them
> include the benefit of the first three patches.  Without those
> patches, the C version takes over 500 seconds and the other take much
> longer too; the relative timings aren't affected much, it's just all
> slower:
> 
> C:                                 106.8s (0.135s ... 1.043s)
> ddlog before optimization patches: 176.0s (0.128s ... 1.804s)
> ddlog after optimization patches:   35.2s (0.129s ... 0.147s)
> 
> (I haven't re-measured for v3.)
> 
> v1->v2:
>    - Don't remove --output-only-table option from ovsdb2ddlog2c
>      in "ovn-northd-ddlog: Intern selected input relations.".
>    - New patches "ovn-nbctl: Daemon mode is no longer experimental."
>      and "ovn-nbctl: Recommend ovn-appctl instead of ovs-appctl."
>      and make similar changes to new ovn-sbctl manpage.
>    - Update ovn-sbctl and ovn-nbctl manpages to reference ovn-appctl
>      manpage.
>    - Various trivial changes suggested by checkpatch.
>    - New patches "ovn-nbctl: Fix memory leak in client mode."
>      and "ovn-northd-ddlog: Fix two memory leaks." fix memory leaks
>      reported by Numan and found by Address Sanitizer.
>    - Fix bug introduced into ovsdb2ddlog2c in "ovn-northd-ddlog: Intern
>      selected input relations."
> 
> v2->v3:
>    - Rebase.
>    - Apply Mark Michelson's comments for "ovn-sbctl: Add daemon support."
>      and to "tests: Miscellaneous debuggability improvements.".
>    - New patch "ovn-nbctl: Don't replicate entire database unnecessarily."
> 
> Ben Pfaff (12):
>    ovn-northd-ddlog: Fix two memory leaks.
>    ovn-nbctl: Fix memory leak in client mode.
>    ovn-nbctl: Improve manpage.
>    ovn-nbctl: Recommend ovn-appctl instead of ovs-appctl.
>    ovn-nbctl: Daemon mode is no longer experimental.
>    ovn-nbctl: Refactor into infrastructure and northbound details.
>    ovn-dbctl: Fix memory leak in client mode.
>    ovn-sbctl: Add daemon support.
>    ovn-nbctl: Don't replicate entire database unnecessarily.
>    tests: Miscellaneous debuggability improvements.
>    ovn-northd-ddlog: Preserve NB_Global more carefully.
>    tutorial: Add benchmarking test script to run within sandbox.
> 
> Leonid Ryzhyk (15):
>    ovn-northd-ddlog: Upgrade to ddlog 0.38.
>    ovn-northd-ddlog: Remove `lr` field from `Router`.
>    ovn-northd-ddlog: Intern the `Router` table.
>    ovn-northd-ddlog: Workaround for slow group_by.
>    ovn-northd-ddlog: Intern the Switch table.
>    ovn-northd-ddlog: Remove `ls` field from `Switch`.
>    ovn-northd-ddlog: Intern the SwitchPort table.
>    ovn-northd-ddlog: Intern the RouterPort table.
>    ovn-northd-ddlog: Remove unused function.
>    ovn-northd-ddlog: Eliminate remaining Ref's.
>    ovn-northd-ddlog: Eliminate redundant dereferences.
>    ovn-northd-ddlog: Intern selected input relations.
>    ovn-northd-ddlog: Intern nb::Logical_Router_Port.
>    ovn-northd-ddlog: Intern nb::Logical_Switch_Port.
>    ovn-northd-ddlog: Remove Router.static_routes.
> 
>   NEWS                          |    5 +-
>   manpages.mk                   |   17 -
>   northd/helpers.dl             |   40 +-
>   northd/ipam.dl                |   61 +-
>   northd/lrouter.dl             |  188 +--
>   northd/lswitch.dl             |  243 ++--
>   northd/multicast.dl           |   77 +-
>   northd/ovn-nb.dlopts          |   10 +
>   northd/ovn-northd-ddlog.c     |   23 +-
>   northd/ovn-sb.dlopts          |    1 +
>   northd/ovn_northd.dl          | 1045 ++++++++-------
>   northd/ovsdb2ddlog2c          |    4 +-
>   tests/ovn-sbctl.at            |   76 +-
>   tests/ovn.at                  |   51 +-
>   tutorial/automake.mk          |    3 +-
>   tutorial/northd_ddlog_test.sh |   81 ++
>   utilities/automake.mk         |   12 +-
>   utilities/ovn-dbctl.c         | 1230 +++++++++++++++++
>   utilities/ovn-dbctl.h         |   61 +
>   utilities/ovn-nbctl.8.xml     |  667 +++++----
>   utilities/ovn-nbctl.c         | 2385 ++++++++++++++-------------------
>   utilities/ovn-sbctl.8.in      |  317 -----
>   utilities/ovn-sbctl.8.xml     |  580 ++++++++
>   utilities/ovn-sbctl.c         |  669 ++-------
>   24 files changed, 4464 insertions(+), 3382 deletions(-)
>   create mode 100755 tutorial/northd_ddlog_test.sh
>   create mode 100644 utilities/ovn-dbctl.c
>   create mode 100644 utilities/ovn-dbctl.h
>   delete mode 100644 utilities/ovn-sbctl.8.in
>   create mode 100644 utilities/ovn-sbctl.8.xml
>