| Message ID | 20210507040659.26830-1-blp@ovn.org |
|---|---|
| Headers | show |
| Series | ddlog 5x performance improvement | expand |
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@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@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 >
On Thu, May 20, 2021 at 04:50:58PM -0400, Mark Michelson wrote: > 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@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@redhat.com> > > but I wouldn't be averse to others having a look to be certain. Thanks. I've now applied the whole series, except the last patch, to master. Somehow I omitted the configure-time check patch. I'll post it again.
On Thu, May 20, 2021 at 10:23:36PM -0700, Ben Pfaff wrote: > On Thu, May 20, 2021 at 04:50:58PM -0400, Mark Michelson wrote: > > 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@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@redhat.com> > > > > but I wouldn't be averse to others having a look to be certain. > > Thanks. I've now applied the whole series, except the last patch, to > master. > > Somehow I omitted the configure-time check patch. I'll post it again. It's a 3-patch series now: https://mail.openvswitch.org/pipermail/ovs-dev/2021-May/383261.html https://mail.openvswitch.org/pipermail/ovs-dev/2021-May/383259.html https://mail.openvswitch.org/pipermail/ovs-dev/2021-May/383260.html
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