Looks good to me.  I might be inclined to use camel case for the new
assign_c_value function, since it's siblings are written that way. It's
fine as is though.

Acked-by: Ethan Jackson <et...@nicira.com>



On Tue, Mar 5, 2013 at 4:28 PM, Ben Pfaff <b...@nicira.com> wrote:

> For 1000 tunnels with CFM enabled, this reduces CPU use from
> about 36% to about 30%.
>
> Signed-off-by: Ben Pfaff <b...@nicira.com>
> ---
>  lib/ovsdb-idl.c        |   46
> +++++++++++++++++++++++++++++++++++-----------
>  lib/ovsdb-idl.h        |    5 ++++-
>  ovsdb/ovsdb-idlc.in    |   40 +++++++++++++++++++++++++++++++---------
>  python/ovs/db/types.py |   11 ++++++++++-
>  4 files changed, 80 insertions(+), 22 deletions(-)
>
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index fbf4bba..0c644a5 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1826,10 +1826,10 @@ ovsdb_idl_txn_complete(struct ovsdb_idl_txn *txn,
>   * Takes ownership of what 'datum' points to (and in some cases destroys
> that
>   * data before returning) but makes a copy of 'datum' itself.  (Commonly
>   * 'datum' is on the caller's stack.) */
> -void
> -ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
> -                    const struct ovsdb_idl_column *column,
> -                    struct ovsdb_datum *datum)
> +static void
> +ovsdb_idl_txn_write__(const struct ovsdb_idl_row *row_,
> +                      const struct ovsdb_idl_column *column,
> +                      struct ovsdb_datum *datum, bool owns_datum)
>  {
>      struct ovsdb_idl_row *row = CONST_CAST(struct ovsdb_idl_row *, row_);
>      const struct ovsdb_idl_table_class *class;
> @@ -1837,8 +1837,7 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
>      bool write_only;
>
>      if (ovsdb_idl_row_is_synthetic(row)) {
> -        ovsdb_datum_destroy(datum, &column->type);
> -        return;
> +        goto discard_datum;
>      }
>
>      class = row->table->class;
> @@ -1853,8 +1852,7 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
>      if (row->table->idl->verify_write_only && !write_only) {
>          VLOG_ERR("Bug: Attempt to write to a read/write column (%s:%s)
> when"
>                   " explicitly configured not to.", class->name,
> column->name);
> -        ovsdb_datum_destroy(datum, &column->type);
> -        return;
> +        goto discard_datum;
>      }
>
>      /* If this is a write-only column and the datum being written is the
> same
> @@ -1870,8 +1868,7 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row *row_,
>       * ovsdb_idl_txn_commit().) */
>      if (write_only && ovsdb_datum_equals(ovsdb_idl_read(row, column),
>                                           datum, &column->type)) {
> -        ovsdb_datum_destroy(datum, &column->type);
> -        return;
> +        goto discard_datum;
>      }
>
>      if (hmap_node_is_null(&row->txn_node)) {
> @@ -1889,9 +1886,36 @@ ovsdb_idl_txn_write(const struct ovsdb_idl_row
> *row_,
>      } else {
>          bitmap_set1(row->written, column_idx);
>      }
> -    row->new[column_idx] = *datum;
> +    if (owns_datum) {
> +        row->new[column_idx] = *datum;
> +    } else {
> +        ovsdb_datum_clone(&row->new[column_idx], datum, &column->type);
> +    }
>      (column->unparse)(row);
>      (column->parse)(row, &row->new[column_idx]);
> +    return;
> +
> +discard_datum:
> +    if (owns_datum) {
> +        ovsdb_datum_destroy(datum, &column->type);
> +    }
> +}
> +
> +void
> +ovsdb_idl_txn_write(const struct ovsdb_idl_row *row,
> +                    const struct ovsdb_idl_column *column,
> +                    struct ovsdb_datum *datum)
> +{
> +    ovsdb_idl_txn_write__(row, column, datum, true);
> +}
> +
> +void
> +ovsdb_idl_txn_write_clone(const struct ovsdb_idl_row *row,
> +                          const struct ovsdb_idl_column *column,
> +                          const struct ovsdb_datum *datum)
> +{
> +    ovsdb_idl_txn_write__(row, column,
> +                          CONST_CAST(struct ovsdb_datum *, datum), false);
>  }
>
>  /* Causes the original contents of 'column' in 'row_' to be verified as a
> diff --git a/lib/ovsdb-idl.h b/lib/ovsdb-idl.h
> index 27008b7..b428501 100644
> --- a/lib/ovsdb-idl.h
> +++ b/lib/ovsdb-idl.h
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> +/* Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -202,6 +202,9 @@ const struct uuid *ovsdb_idl_txn_get_insert_uuid(const
> struct ovsdb_idl_txn *,
>  void ovsdb_idl_txn_write(const struct ovsdb_idl_row *,
>                           const struct ovsdb_idl_column *,
>                           struct ovsdb_datum *);
> +void ovsdb_idl_txn_write_clone(const struct ovsdb_idl_row *,
> +                               const struct ovsdb_idl_column *,
> +                               const struct ovsdb_datum *);
>  void ovsdb_idl_txn_delete(const struct ovsdb_idl_row *);
>  const struct ovsdb_idl_row *ovsdb_idl_txn_insert(
>      struct ovsdb_idl_txn *, const struct ovsdb_idl_table_class *,
> diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
> index dc0839e..1f21950 100755
> --- a/ovsdb/ovsdb-idlc.in
> +++ b/ovsdb/ovsdb-idlc.in
> @@ -517,34 +517,54 @@ void
>              print "{"
>              print "    struct ovsdb_datum datum;"
>              if type.n_min == 1 and type.n_max == 1:
> +                print "    union ovsdb_atom key;"
> +                if type.value:
> +                    print "    union ovsdb_atom value;"
>                  print
>                  print "    ovs_assert(inited);"
>                  print "    datum.n = 1;"
> -                print "    datum.keys = xmalloc(sizeof *datum.keys);"
> -                print "    " + type.key.copyCValue("datum.keys[0].%s" %
> type.key.type.to_string(), keyVar)
> +                print "    datum.keys = &key;"
> +                print "    " +
> type.key.assign_c_value_casting_away_const("key.%s" %
> type.key.type.to_string(), keyVar)
>                  if type.value:
> -                    print "    datum.values = xmalloc(sizeof
> *datum.values);"
> -                    print "    "+
> type.value.copyCValue("datum.values[0].%s" % type.value.type.to_string(),
> valueVar)
> +                    print "    datum.values = &value;"
> +                    print "    "+
> type.value.assign_c_value_casting_away_const("value.%s" %
> type.value.type.to_string(), valueVar)
>                  else:
>                      print "    datum.values = NULL;"
> +                txn_write_func = "ovsdb_idl_txn_write_clone"
>              elif type.is_optional_pointer():
> +                print "    union ovsdb_atom key;"
>                  print
>                  print "    ovs_assert(inited);"
>                  print "    if (%s) {" % keyVar
>                  print "        datum.n = 1;"
> -                print "        datum.keys = xmalloc(sizeof *datum.keys);"
> -                print "        " + type.key.copyCValue("datum.keys[0].%s"
> % type.key.type.to_string(), keyVar)
> +                print "        datum.keys = &key;"
> +                print "        " +
> type.key.assign_c_value_casting_away_const("key.%s" %
> type.key.type.to_string(), keyVar)
> +                print "    } else {"
> +                print "        datum.n = 0;"
> +                print "        datum.keys = NULL;"
> +                print "    }"
> +                print "    datum.values = NULL;"
> +                txn_write_func = "ovsdb_idl_txn_write_clone"
> +            elif type.n_max == 1:
> +                print "    union ovsdb_atom key;"
> +                print
> +                print "    ovs_assert(inited);"
> +                print "    if (%s) {" % nVar
> +                print "        datum.n = 1;"
> +                print "        datum.keys = &key;"
> +                print "        " +
> type.key.assign_c_value_casting_away_const("key.%s" %
> type.key.type.to_string(), "*" + keyVar)
>                  print "    } else {"
>                  print "        datum.n = 0;"
>                  print "        datum.keys = NULL;"
>                  print "    }"
>                  print "    datum.values = NULL;"
> +                txn_write_func = "ovsdb_idl_txn_write_clone"
>              else:
>                  print "    size_t i;"
>                  print
>                  print "    ovs_assert(inited);"
>                  print "    datum.n = %s;" % nVar
> -                print "    datum.keys = xmalloc(%s * sizeof
> *datum.keys);" % nVar
> +                print "    datum.keys = %s ? xmalloc(%s * sizeof
> *datum.keys) : NULL;" % (nVar, nVar)
>                  if type.value:
>                      print "    datum.values = xmalloc(%s * sizeof
> *datum.values);" % nVar
>                  else:
> @@ -560,8 +580,10 @@ void
>                      valueType = "OVSDB_TYPE_VOID"
>                  print "    ovsdb_datum_sort_unique(&datum, %s, %s);" % (
>                      type.key.toAtomicType(), valueType)
> -            print "    ovsdb_idl_txn_write(&row->header_,
> &%(s)s_columns[%(S)s_COL_%(C)s], &datum);" \
> -                % {'s': structName,
> +                txn_write_func = "ovsdb_idl_txn_write"
> +            print "    %(f)s(&row->header_,
> &%(s)s_columns[%(S)s_COL_%(C)s], &datum);" \
> +                % {'f': txn_write_func,
> +                   's': structName,
>                     'S': structName.upper(),
>                     'C': columnName.upper()}
>              print "}"
> diff --git a/python/ovs/db/types.py b/python/ovs/db/types.py
> index bd1c259..c858471 100644
> --- a/python/ovs/db/types.py
> +++ b/python/ovs/db/types.py
> @@ -1,4 +1,4 @@
> -# Copyright (c) 2009, 2010, 2011, 2012 Nicira, Inc.
> +# Copyright (c) 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
>  #
>  # Licensed under the Apache License, Version 2.0 (the "License");
>  # you may not use this file except in compliance with the License.
> @@ -353,6 +353,15 @@ class BaseType(object):
>          else:
>              return "%(dst)s = %(src)s;" % args
>
> +    def assign_c_value_casting_away_const(self, dst, src):
> +        args = {'dst': dst, 'src': src}
> +        if self.ref_table_name:
> +            return ("%(dst)s = %(src)s->header_.uuid;") % args
> +        elif self.type == StringType:
> +            return "%(dst)s = CONST_CAST(char *, %(src)s);" % args
> +        else:
> +            return "%(dst)s = %(src)s;" % args
> +
>      def initCDefault(self, var, is_optional):
>          if self.ref_table_name:
>              return "%s = NULL;" % var
> --
> 1.7.2.5
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to