[ovs-dev] [PATCH ovn 09/21] ovn-northd-ddlog: Workaround for slow group_by.

Ben Pfaff blp at ovn.org
Sat Mar 27 00:31:35 UTC 2021


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

diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 80d8598bd7dc..5a7a11295964 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -712,11 +712,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),
@@ -738,11 +737,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),
@@ -771,9 +769,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 at nb::Logical_Switch_Port{._uuid = port_uuid,
                                                    .name = port_name},
                 .sw = &Switch{.ls = nb::Logical_Switch{._uuid = ls_uuid}}),
-- 
2.29.2



More information about the dev mailing list