[ovs-dev,v7,1/5] ovn-northd-ddlog: Optimize AggregatedFlow rules.

From: Ben Pfaff <blp@ovn.org>

This should avoid some work by doing the cheapest check (the one on
UseLogicalDatapathGroups) before any joins.  DDlog is probably
factoring out the reference to the Flow relation, which is identical
in both, but this ought to avoid the group_by aggregation (which is
relatively expensive) in the case where UseLogicalDatapathGroups is
not enabled.

Signed-off-by: Ben Pfaff <blp@ovn.org>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
 northd/ovn_northd.dl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Wed, Jul 14, 2021 at 1:34 AM Lorenzo Bianconi <
lorenzo.bianconi@redhat.com> wrote:
>
> From: Ben Pfaff <blp@ovn.org>
>
> This should avoid some work by doing the cheapest check (the one on
> UseLogicalDatapathGroups) before any joins.  DDlog is probably
> factoring out the reference to the Flow relation, which is identical
> in both, but this ought to avoid the group_by aggregation (which is
> relatively expensive) in the case where UseLogicalDatapathGroups is
> not enabled.
>
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---
>  northd/ovn_northd.dl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index ceeabe6f3..46280e22e 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1659,17 +1659,17 @@ AggregatedFlow(.logical_datapaths = g.to_set(),
>                 .__match = __match,
>                 .actions = actions,
>                 .external_ids = external_ids) :-
> +    UseLogicalDatapathGroups[true],
>      Flow(logical_datapath, stage, priority, __match, actions,
external_ids),
> -    var g = logical_datapath.group_by((stage, priority, __match,
actions, external_ids)),
> -    UseLogicalDatapathGroups[true].
> +    var g = logical_datapath.group_by((stage, priority, __match,
actions, external_ids)).
>  AggregatedFlow(.logical_datapaths = set_singleton(logical_datapath),
>                 .stage = stage,
>                 .priority = priority,
>                 .__match = __match,
>                 .actions = actions,
>                 .external_ids = external_ids) :-
> -    Flow(logical_datapath, stage, priority, __match, actions,
external_ids),
> -    UseLogicalDatapathGroups[false].
> +    UseLogicalDatapathGroups[false],
> +    Flow(logical_datapath, stage, priority, __match, actions,
external_ids).
>
>  for (f in AggregatedFlow()) {
>      var pipeline = if (f.stage.pipeline == Ingress) "ingress" else
"egress" in
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Thanks! I didn't know that the order matters. (not sure if there is
documentation that I missed)

Acked-by: Han Zhou <hzhou@ovn.org>

On Thu, Jul 15, 2021 at 11:18:12AM -0700, Han Zhou wrote:
> On Wed, Jul 14, 2021 at 1:34 AM Lorenzo Bianconi <
> lorenzo.bianconi@redhat.com> wrote:
> > This should avoid some work by doing the cheapest check (the one on
> > UseLogicalDatapathGroups) before any joins.  DDlog is probably
> > factoring out the reference to the Flow relation, which is identical
> > in both, but this ought to avoid the group_by aggregation (which is
> > relatively expensive) in the case where UseLogicalDatapathGroups is
> > not enabled.
> 
> Thanks! I didn't know that the order matters. (not sure if there is
> documentation that I missed)

In general, DDlog executes each rule in the order given, so if you have
a series of joins in a rule, then it's a good idea to order them, if you
can, to keep the number of records small at each join step.  It won't
affect the correctness, but it will affect performance.

This might not be documented well.  I do occasionally work on the DDlog
documentation, so I've made a note to try to see whether the effect of
ordering is mentioned and improve it if I can.

In this particular case, I talked to Leonid about it during a discussion
of how to improve performance and memory use in the benchmark currently
at issue, and Leonid says that the ordering doesn't actually matter in
this case because both of the possibilities (the ones for
UseLogicalDatapathGroups[true] and UseLogicalDatapathGroups[false]) had
an identical clause at the beginning.  DDlog optimizes identical
prefixes, so there was no real difference in performance.

This patch could, therefore, be dropped, but it should also not be
harmful.  Dropping it would require some changes to the later patch that
updates the ovn-northd ddlog code, so folding it into that one would
also be an option.

> On Thu, Jul 15, 2021 at 11:18:12AM -0700, Han Zhou wrote:
> > On Wed, Jul 14, 2021 at 1:34 AM Lorenzo Bianconi <
> > lorenzo.bianconi@redhat.com> wrote:
> > > This should avoid some work by doing the cheapest check (the one on
> > > UseLogicalDatapathGroups) before any joins.  DDlog is probably
> > > factoring out the reference to the Flow relation, which is identical
> > > in both, but this ought to avoid the group_by aggregation (which is
> > > relatively expensive) in the case where UseLogicalDatapathGroups is
> > > not enabled.
> > 
> > Thanks! I didn't know that the order matters. (not sure if there is
> > documentation that I missed)
> 
> In general, DDlog executes each rule in the order given, so if you have
> a series of joins in a rule, then it's a good idea to order them, if you
> can, to keep the number of records small at each join step.  It won't
> affect the correctness, but it will affect performance.
> 
> This might not be documented well.  I do occasionally work on the DDlog
> documentation, so I've made a note to try to see whether the effect of
> ordering is mentioned and improve it if I can.
> 
> In this particular case, I talked to Leonid about it during a discussion
> of how to improve performance and memory use in the benchmark currently
> at issue, and Leonid says that the ordering doesn't actually matter in
> this case because both of the possibilities (the ones for
> UseLogicalDatapathGroups[true] and UseLogicalDatapathGroups[false]) had
> an identical clause at the beginning.  DDlog optimizes identical
> prefixes, so there was no real difference in performance.
> 
> This patch could, therefore, be dropped, but it should also not be
> harmful.  Dropping it would require some changes to the later patch that
> updates the ovn-northd ddlog code, so folding it into that one would
> also be an option.
> 

Do you want me to repost dropping this patch or is the series fine as it is?

Regards,
Lorenzo

On Thu, Jul 15, 2021 at 08:45:00PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Jul 15, 2021 at 11:18:12AM -0700, Han Zhou wrote:
> > > On Wed, Jul 14, 2021 at 1:34 AM Lorenzo Bianconi <
> > > lorenzo.bianconi@redhat.com> wrote:
> > > > This should avoid some work by doing the cheapest check (the one on
> > > > UseLogicalDatapathGroups) before any joins.  DDlog is probably
> > > > factoring out the reference to the Flow relation, which is identical
> > > > in both, but this ought to avoid the group_by aggregation (which is
> > > > relatively expensive) in the case where UseLogicalDatapathGroups is
> > > > not enabled.
> > > 
> > > Thanks! I didn't know that the order matters. (not sure if there is
> > > documentation that I missed)
> > 
> > In general, DDlog executes each rule in the order given, so if you have
> > a series of joins in a rule, then it's a good idea to order them, if you
> > can, to keep the number of records small at each join step.  It won't
> > affect the correctness, but it will affect performance.
> > 
> > This might not be documented well.  I do occasionally work on the DDlog
> > documentation, so I've made a note to try to see whether the effect of
> > ordering is mentioned and improve it if I can.
> > 
> > In this particular case, I talked to Leonid about it during a discussion
> > of how to improve performance and memory use in the benchmark currently
> > at issue, and Leonid says that the ordering doesn't actually matter in
> > this case because both of the possibilities (the ones for
> > UseLogicalDatapathGroups[true] and UseLogicalDatapathGroups[false]) had
> > an identical clause at the beginning.  DDlog optimizes identical
> > prefixes, so there was no real difference in performance.
> > 
> > This patch could, therefore, be dropped, but it should also not be
> > harmful.  Dropping it would require some changes to the later patch that
> > updates the ovn-northd ddlog code, so folding it into that one would
> > also be an option.
> > 
> 
> Do you want me to repost dropping this patch or is the series fine as it is?

The series is fine as is.

On Thu, Jul 15, 2021 at 11:29 AM Ben Pfaff <blp@ovn.org> wrote:
>
> On Thu, Jul 15, 2021 at 11:18:12AM -0700, Han Zhou wrote:
> > On Wed, Jul 14, 2021 at 1:34 AM Lorenzo Bianconi <
> > lorenzo.bianconi@redhat.com> wrote:
> > > This should avoid some work by doing the cheapest check (the one on
> > > UseLogicalDatapathGroups) before any joins.  DDlog is probably
> > > factoring out the reference to the Flow relation, which is identical
> > > in both, but this ought to avoid the group_by aggregation (which is
> > > relatively expensive) in the case where UseLogicalDatapathGroups is
> > > not enabled.
> >
> > Thanks! I didn't know that the order matters. (not sure if there is
> > documentation that I missed)
>
> In general, DDlog executes each rule in the order given, so if you have
> a series of joins in a rule, then it's a good idea to order them, if you
> can, to keep the number of records small at each join step.  It won't
> affect the correctness, but it will affect performance.
>
> This might not be documented well.  I do occasionally work on the DDlog
> documentation, so I've made a note to try to see whether the effect of
> ordering is mentioned and improve it if I can.
>
> In this particular case, I talked to Leonid about it during a discussion
> of how to improve performance and memory use in the benchmark currently
> at issue, and Leonid says that the ordering doesn't actually matter in
> this case because both of the possibilities (the ones for
> UseLogicalDatapathGroups[true] and UseLogicalDatapathGroups[false]) had
> an identical clause at the beginning.  DDlog optimizes identical
> prefixes, so there was no real difference in performance.
>
Thanks for the explanation. However, the group_by part is not common for
these two, would this reordering still save that cost when
UseLogicalDatapathGroups is disabled?

> This patch could, therefore, be dropped, but it should also not be
> harmful.  Dropping it would require some changes to the later patch that
> updates the ovn-northd ddlog code, so folding it into that one would
> also be an option.

> On Thu, Jul 15, 2021 at 08:45:00PM +0200, Lorenzo Bianconi wrote:
> > > On Thu, Jul 15, 2021 at 11:18:12AM -0700, Han Zhou wrote:
> > > > On Wed, Jul 14, 2021 at 1:34 AM Lorenzo Bianconi <
> > > > lorenzo.bianconi@redhat.com> wrote:
> > > > > This should avoid some work by doing the cheapest check (the one on
> > > > > UseLogicalDatapathGroups) before any joins.  DDlog is probably
> > > > > factoring out the reference to the Flow relation, which is identical
> > > > > in both, but this ought to avoid the group_by aggregation (which is
> > > > > relatively expensive) in the case where UseLogicalDatapathGroups is
> > > > > not enabled.
> > > > 
> > > > Thanks! I didn't know that the order matters. (not sure if there is
> > > > documentation that I missed)
> > > 
> > > In general, DDlog executes each rule in the order given, so if you have
> > > a series of joins in a rule, then it's a good idea to order them, if you
> > > can, to keep the number of records small at each join step.  It won't
> > > affect the correctness, but it will affect performance.
> > > 
> > > This might not be documented well.  I do occasionally work on the DDlog
> > > documentation, so I've made a note to try to see whether the effect of
> > > ordering is mentioned and improve it if I can.
> > > 
> > > In this particular case, I talked to Leonid about it during a discussion
> > > of how to improve performance and memory use in the benchmark currently
> > > at issue, and Leonid says that the ordering doesn't actually matter in
> > > this case because both of the possibilities (the ones for
> > > UseLogicalDatapathGroups[true] and UseLogicalDatapathGroups[false]) had
> > > an identical clause at the beginning.  DDlog optimizes identical
> > > prefixes, so there was no real difference in performance.
> > > 
> > > This patch could, therefore, be dropped, but it should also not be
> > > harmful.  Dropping it would require some changes to the later patch that
> > > updates the ovn-northd ddlog code, so folding it into that one would
> > > also be an option.
> > > 
> > 
> > Do you want me to repost dropping this patch or is the series fine as it is?
> 
> The series is fine as is.
> 

ack, thx for the clarification.

Regards,
Lorenzo

On 7/14/21 10:33 AM, Lorenzo Bianconi wrote:
> From: Ben Pfaff <blp@ovn.org>
> 
> This should avoid some work by doing the cheapest check (the one on
> UseLogicalDatapathGroups) before any joins.  DDlog is probably
> factoring out the reference to the Flow relation, which is identical
> in both, but this ought to avoid the group_by aggregation (which is
> relatively expensive) in the case where UseLogicalDatapathGroups is
> not enabled.
> 
> Signed-off-by: Ben Pfaff <blp@ovn.org>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---

Acked-by: Dumitru Ceara <dceara@redhat.com>

Thanks,
Dumitru