[ovs-dev] [PATCH] ovsdb: Weak references performance fix

[Ben Pfaff]

On Mon, Jun 27, 2016 at 08:07:02PM +0000, Rodriguez Betancourt, Esteban wrote:
> Prevents the cloning of rows with outgoing or
> incoming weak references when those rows aren't
> being modified.
> 
> It improves the OVSDB Server performance when
> many rows with weak references are involved
> in a transaction.
> 
> Signed-off-by: Esteban Rodriguez Betancourt <estebarb at hpe.com>

Great idea.  Thanks for working on this!

The new ovs_list_transplant() function takes two "const" pointers to
lists but it modifies both lists.  I don't think it makes sense for them
to be const.

The new ovs_list_transplant() function has a name that doesn't give much
of a idea of what it does.  I think that really it appends everything in
'src' to 'dst', although those are not great names since both lists are
modified.  Maybe ovs_list_push_back_all() would be a better name.

I think that the new ovs_list_transplant() function can be implemented
in terms of ovs_list_splice(), as:
        ovs_list_splice(dst, &src->next, src);
I see a couple of existing uses of ovs_list_splice() to do that, so it's
probably best to convert them to use the new function for consistency.

Since add_weak_ref() doesn't use its first argument now, please delete
the parameter entirely instead of marking it OVS_UNUSED.

In add_weak_ref(), instead of using memcpy() to copy a struct uuid,
please use an ordinary assignment with =.

I don't think that this loop in ovsdb_txn_update_weak_refs() needs to be
the _SAFE variant: it appears to me that it iterates on ->src_refs and
->src_node but only modifies ->dst_refs and ->dst_node:
> +        LIST_FOR_EACH_SAFE (weak, next, src_node, &txn_row->new->src_refs) {
> +            /* dst_row MUST exist */
> +            dst_row = CONST_CAST(struct ovsdb_row *,
> +                    ovsdb_table_get_row(weak->dst_table, &weak->dst));
> +            ovs_list_insert(&dst_row->dst_refs, &weak->dst_node);
> +        }

It's taking me some thought to convince myself that this new version is
as correct as the previous version.  Do you have any arguments or
explanations to help me out?

Thanks,

Ben.