diff mbox series

[ovs-dev,v3,15/27] ovn-northd-ddlog: Workaround for slow group_by.

Message ID 20210507040659.26830-16-blp@ovn.org
State Accepted
Headers show
Series ddlog 5x performance improvement | expand

Commit Message

Ben Pfaff May 7, 2021, 4:06 a.m. UTC
From: Leonid Ryzhyk <lryzhyk@vmware.com>

This patch is a workaround for a performance issue in the DDlog
compiler.  The issue will hopefully be resolved in a future version of
DDlog, but for now we need this and possibly a few other similar fixes.

Here is one affected rule:

```
sb::Out_Port_Group(._uuid = hash128(sb_name), .name = sb_name, .ports = port_names) :-
    nb::Port_Group(._uuid = _uuid, .name = nb_name, .ports = pg_ports),
    var port_uuid = FlatMap(pg_ports),
    &SwitchPort(.lsp = lsp@nb::Logical_Switch_Port{._uuid = port_uuid,
                                                   .name = port_name},
                .sw = &Switch{.ls = nb::Logical_Switch{._uuid = ls_uuid}}),
    TunKeyAllocation(.datapath = ls_uuid, .tunkey = tunkey),
    var sb_name = "${tunkey}_${nb_name}",
    var port_names = port_name.group_by((_uuid, sb_name)).to_set().
```

The first literal in the body of the rule binds variable `pg_ports` to
the array of ports in the port group.  This is a potentially large
array that immediately gets flattened by the `FlatMap` operator.
Since the `pg_ports` variable is not used in the remainder of the rule,
DDlog normally would not propagate it through the rest of the rule.
Unfortunately, due to a subtle semantic quirk, the behavior is different
when there is a `group_by` operator further down in the rule, in which
case unused variables are still propagated through the rule, which
involves expensive copies.

The workaround I implemented factors the first two terms in the
rule into a separate `PortGroupPort` relation, so that the ports array
no longer occurs in the new version of the rule:

```
sb::Out_Port_Group(._uuid = hash128(sb_name), .name = sb_name, .ports = port_names) :-
    PortGroupPort(.pg_uuid = _uuid, .pg_name = nb_name, .port = port_uuid),
    &SwitchPort(.lsp = lsp@nb::Logical_Switch_Port{._uuid = port_uuid,
                                                   .name = port_name},
                .sw = &Switch{.ls = nb::Logical_Switch{._uuid = ls_uuid}}),
    TunKeyAllocation(.datapath = ls_uuid, .tunkey = tunkey),
    var sb_name = "${tunkey}_${nb_name}",
    var port_names = port_name.group_by((_uuid, sb_name)).to_set().
```

Again, benchmarking is likely to reveal more instances of this.  A
proper fix will require a change to the DDlog compiler.

Signed-off-by: Leonid Ryzhyk <lryzhyk@vmware.com>
Signed-off-by: Ben Pfaff <blp@ovn.org>
---
 northd/ovn_northd.dl | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

0-day Robot May 7, 2021, 5:10 a.m. UTC | #1
Bleep bloop.  Greetings Ben Pfaff, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Ben Pfaff <blp@ovn.org>
Lines checked: 111, Warnings: 1, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 51bbc832b7b6..50203de88867 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -720,11 +720,10 @@  sb::Out_Address_Set(._uuid = hash128("svc_monitor_mac"),
     SvcMonitorMac(svc_monitor_mac).
 
 sb::Out_Address_Set(hash128(as_name), as_name, pg_ip4addrs.union()) :-
-    nb::Port_Group(.ports = pg_ports, .name = pg_name),
+    PortGroupPort(.pg_name = pg_name, .port = port_uuid),
     var as_name = pg_name ++ "_ip4",
     // avoid name collisions with user-defined Address_Sets
     not nb::Address_Set(.name = as_name),
-    var port_uuid = FlatMap(pg_ports),
     PortStaticAddresses(.lsport = port_uuid, .ip4addrs = stat),
     SwitchPortNewDynamicAddress(&SwitchPort{.lsp = nb::Logical_Switch_Port{._uuid = port_uuid}},
                                 dyn_addr),
@@ -746,11 +745,10 @@  sb::Out_Address_Set(hash128(as_name), as_name, set_empty()) :-
     not nb::Address_Set(.name = as_name).
 
 sb::Out_Address_Set(hash128(as_name), as_name, pg_ip6addrs.union()) :-
-    nb::Port_Group(.ports = pg_ports, .name = pg_name),
+    PortGroupPort(.pg_name = pg_name, .port = port_uuid),
     var as_name = pg_name ++ "_ip6",
     // avoid name collisions with user-defined Address_Sets
     not nb::Address_Set(.name = as_name),
-    var port_uuid = FlatMap(pg_ports),
     PortStaticAddresses(.lsport = port_uuid, .ip6addrs = stat),
     SwitchPortNewDynamicAddress(&SwitchPort{.lsp = nb::Logical_Switch_Port{._uuid = port_uuid}},
                                 dyn_addr),
@@ -779,9 +777,18 @@  sb::Out_Address_Set(hash128(as_name), as_name, set_empty()) :-
  * SB Port_Group.name uniqueness constraint, ovn-northd populates the field
  * with the value: <SB.Logical_Datapath.tunnel_key>_<NB.Port_Group.name>.
  */
+
+relation PortGroupPort(
+    pg_uuid: uuid,
+    pg_name: string,
+    port: uuid)
+
+PortGroupPort(pg_uuid, pg_name, port) :-
+    nb::Port_Group(._uuid = pg_uuid, .name = pg_name, .ports = pg_ports),
+    var port = FlatMap(pg_ports).
+
 sb::Out_Port_Group(._uuid = hash128(sb_name), .name = sb_name, .ports = port_names) :-
-    nb::Port_Group(._uuid = _uuid, .name = nb_name, .ports = pg_ports),
-    var port_uuid = FlatMap(pg_ports),
+    PortGroupPort(.pg_uuid = _uuid, .pg_name = nb_name, .port = port_uuid),
     &SwitchPort(.lsp = lsp@nb::Logical_Switch_Port{._uuid = port_uuid,
                                                    .name = port_name},
                 .sw = &Switch{.ls = nb::Logical_Switch{._uuid = ls_uuid}}),