On 9/23/21 2:13 AM, Ilya Maximets wrote: > ovsdb-server spends a lot of time cloning atoms for various reasons, > e.g. to create a diff of two rows or to clone a row to the transaction. > All atoms, except for strings, contains a simple value that could be > copied in efficient way, but duplicating strings every time has a > significant performance impact. > > Introducing a new reference-counted structure 'ovsdb_atom_string' > that allows to not copy strings every time, but just increase a > reference counter.
Hi Ilya, This is great and definitely improves performance in my tests! > > Changes in ovsdb/ovsdb-idlc.in are a bit brute-force-like, but I > didn't manage to write anything better. And this part is very hard > to understand and debug, so maybe it's better to keep it in this > more or less understandable shape. After spending some time trying to refactor this a bit and after our offline discussion ovsdbc-idlc.in can be improved a bit. Please see the incremental down below. I also have a tiny nit in ovsdb-data.h. With these addressed: Acked-by: Dumitru Ceara <[email protected]> > > This change allows to increase transaction throughput in benchmarks > up to 2x for standalone databases and 3x for clustered databases, i.e. > number of transactions that ovsdb-server can handle per second. > It also noticeably reduces memory consumption of ovsdb-server. > > Next step will be to consolidate this structure with json strings, > so we will not need to duplicate strings while converting database > objects to json and back. > > Signed-off-by: Ilya Maximets <[email protected]> > --- [...] > diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h > index aa035ebad..fa7e0d2fc 100644 > --- a/lib/ovsdb-data.h > +++ b/lib/ovsdb-data.h > @@ -20,6 +20,7 @@ > #include "compiler.h" > #include "ovsdb-types.h" > #include "openvswitch/shash.h" > +#include "util.h" > > #ifdef __cplusplus > extern "C" { > @@ -31,12 +32,33 @@ struct ds; > struct ovsdb_symbol_table; > struct smap; > > +struct ovsdb_atom_string { > + char *string; > + int n_refs; Nit: I'd make this size_t, or unsigned. Regards, Dumitru --- diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in index 877f903cbd5c..04035cb0c272 100755 --- a/ovsdb/ovsdb-idlc.in +++ b/ovsdb/ovsdb-idlc.in @@ -584,13 +584,13 @@ static void print("") print(" if (datum->n >= 1) {") if not type.key.ref_table: - print(" %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_Cvalue_string())) + print(" %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_rvalue_string())) else: print(" %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->keys[0].uuid));" % (keyVar, prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower())) if valueVar: if not type.value.ref_table: - print(" %s = datum->values[0].%s;" % (valueVar, type.value.type.to_Cvalue_string())) + print(" %s = datum->values[0].%s;" % (valueVar, type.value.type.to_rvalue_string())) else: print(" %s = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[0].uuid));" % (valueVar, prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower())) print(" } else {") @@ -618,7 +618,7 @@ static void """ % (prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower(), prefix, type.key.ref_table.name.lower())) keySrc = "keyRow" else: - keySrc = "datum->keys[i].%s" % type.key.type.to_Cvalue_string() + keySrc = "datum->keys[i].%s" % type.key.type.to_rvalue_string() if type.value and type.value.ref_table: print("""\ struct %s%s *valueRow = %s%s_cast(ovsdb_idl_get_row_arc(row_, &%stable_%s, &datum->values[i].uuid)); @@ -628,7 +628,7 @@ static void """ % (prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower(), prefix, type.value.ref_table.name.lower())) valueSrc = "valueRow" elif valueVar: - valueSrc = "datum->values[i].%s" % type.value.type.to_Cvalue_string() + valueSrc = "datum->values[i].%s" % type.value.type.to_rvalue_string() print(" if (!row->n_%s) {" % (columnName)) print(" %s = xmalloc(%s * sizeof *%s);" % ( @@ -942,16 +942,10 @@ void print("") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), keyVar)) if type.value: print(" datum.values = value;") - if type.value.type == StringType: - print(" value->s = ovsdb_atom_string_create(" + valueVar + ");") - else: - print(" "+ type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar)) + print(" "+ type.value.copyCValue("value->%s" % type.value.type.to_lvalue_string(), valueVar)) else: print(" datum.values = NULL;") txn_write_func = "ovsdb_idl_txn_write" @@ -961,10 +955,7 @@ void print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -977,10 +968,7 @@ void print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(*" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), "*" + keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -996,15 +984,9 @@ void else: print(" datum.values = NULL;") print(" for (size_t i = 0; i < %s; i++) {" % nVar) - if type.key.type == StringType: - print(" datum.keys[i].s = ovsdb_atom_string_create(" + keyVar + "[i]);") - else: - print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar)) + print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_lvalue_string(), "%s[i]" % keyVar)) if type.value: - if type.value.type == StringType: - print(" datum.values[i].s = ovsdb_atom_string_create(" + valueVar + "[i]);") - else: - print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar)) + print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_lvalue_string(), "%s[i]" % valueVar)) print(" }") if type.value: valueType = type.value.toAtomicType() @@ -1040,14 +1022,8 @@ void ''' % {'s': structName, 'c': columnName,'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.value.to_const_c_type(prefix), 'S': structName.upper(), 'C': columnName.upper(), 't': tableName}) - if type.key.type == StringType: - print(" datum->keys[0].s = ovsdb_atom_string_create(new_key);") - else: - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_key")) - if type.value.type == StringType: - print(" datum->values[0].s = ovsdb_atom_string_create(new_value);") - else: - print(" "+ type.value.copyCValue("datum->values[0].%s" % type.value.type.to_string(), "new_value")) + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_lvalue_string(), "new_key")) + print(" "+ type.value.copyCValue("datum->values[0].%s" % type.value.type.to_lvalue_string(), "new_value")) print(''' ovsdb_idl_txn_write_partial_map(&row->header_, &%(s)s_col_%(c)s, @@ -1071,10 +1047,7 @@ void ''' % {'s': structName, 'c': columnName,'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.value.to_const_c_type(prefix), 'S': structName.upper(), 'C': columnName.upper(), 't': tableName}) - if type.key.type == StringType: - print(" datum->keys[0].s = ovsdb_atom_string_create(delete_key);") - else: - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_key")) + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_lvalue_string(), "delete_key")) print(''' ovsdb_idl_txn_delete_partial_map(&row->header_, &%(s)s_col_%(c)s, @@ -1100,10 +1073,7 @@ void datum->values = NULL; ''' % {'s': structName, 'c': columnName, 'valtype':column.type.key.to_const_c_type(prefix), 't': tableName}) - if type.key.type == StringType: - print(" datum->keys[0].s = ovsdb_atom_string_create(new_value);") - else: - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_value")) + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_lvalue_string(), "new_value")) print(''' ovsdb_idl_txn_write_partial_set(&row->header_, &%(s)s_col_%(c)s, @@ -1127,10 +1097,7 @@ void ''' % {'s': structName, 'c': columnName,'coltype':column.type.key.to_const_c_type(prefix), 'valtype':column.type.key.to_const_c_type(prefix), 'S': structName.upper(), 'C': columnName.upper(), 't': tableName}) - if type.key.type == StringType: - print(" datum->keys[0].s = ovsdb_atom_string_create(delete_value);") - else: - print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_value")) + print(" "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_lvalue_string(), "delete_value")) print(''' ovsdb_idl_txn_delete_partial_set(&row->header_, &%(s)s_col_%(c)s, @@ -1204,16 +1171,9 @@ void print("") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar, refTable=False)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), keyVar, refTable=False)) if type.value: - print(" datum.values = value;") - if type.value.type == StringType: - print(" value->s = ovsdb_atom_string_create(" + valueVar + ");") - else: - print(" "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar, refTable=False)) + print(" "+ type.value.copyCValue("value.%s" % type.value.type.to_lvalue_string(), valueVar, refTable=False)) else: print(" datum.values = NULL;") elif type.is_optional_pointer(): @@ -1222,10 +1182,7 @@ void print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar, refTable=False)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), keyVar, refTable=False)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1237,10 +1194,7 @@ void print(" union ovsdb_atom *key = xmalloc(sizeof *key);") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(*" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar, refTable=False)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), "*" + keyVar, refTable=False)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1254,15 +1208,9 @@ void else: print(" datum.values = NULL;") print(" for (size_t i = 0; i < %s; i++) {" % nVar) - if type.key.type == StringType: - print(" datum.keys[i].s = ovsdb_atom_string_create(" + keyVar + "[i]);") - else: - print(" " + type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar, refTable=False)) + print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_lvalue_string(), "%s[i]" % keyVar, refTable=False)) if type.value: - if type.value.type == StringType: - print(" datum.values[i].s = ovsdb_atom_string_create(" + valueVar + "[i]);") - else: - print(" " + type.value.assign_c_value_casting_away_const("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar, refTable=False)) + print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_lvalue_string(), "%s[i]" % valueVar, refTable=False)) print(" }") if type.value: valueType = type.value.toAtomicType() @@ -1430,16 +1378,10 @@ struct %(s)s * print() print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), keyVar)) if type.value: print(" datum.values = value;") - if type.value.type == StringType: - print(" value->s = ovsdb_atom_string_create(" + valueVar + ");") - else: - print(" " + type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar)) + print(" " + type.value.copyCValue("value->%s" % type.value.type.to_lvalue_string(), valueVar)) else: print(" datum.values = NULL;") txn_write_func = "ovsdb_idl_index_write" @@ -1450,10 +1392,7 @@ struct %(s)s * print(" key = xmalloc(sizeof (union ovsdb_atom));") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), keyVar)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1467,10 +1406,7 @@ struct %(s)s * print(" key = xmalloc(sizeof(union ovsdb_atom));") print(" datum.n = 1;") print(" datum.keys = key;") - if type.key.type == StringType: - print(" key->s = ovsdb_atom_string_create(*" + keyVar + ");") - else: - print(" " + type.key.assign_c_value_casting_away_const("key->%s" % type.key.type.to_string(), "*" + keyVar)) + print(" " + type.key.copyCValue("key->%s" % type.key.type.to_lvalue_string(), "*" + keyVar)) print(" } else {") print(" datum.n = 0;") print(" datum.keys = NULL;") @@ -1487,15 +1423,9 @@ struct %(s)s * else: print(" datum.values = NULL;") print(" for (i = 0; i < %s; i++) {" % nVar) - if type.key.type == StringType: - print(" datum.keys[i].s = ovsdb_atom_string_create(" + keyVar + "[i]);") - else: - print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar)) + print(" " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_lvalue_string(), "%s[i]" % keyVar)) if type.value: - if type.value.type == StringType: - print(" datum.values[i].s = ovsdb_atom_string_create(" + valueVar + "[i]);") - else: - print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar)) + print(" " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_lvalue_string(), "%s[i]" % valueVar)) print(" }") if type.value: valueType = type.value.toAtomicType() diff --git a/python/ovs/db/types.py b/python/ovs/db/types.py index 18485f701258..0b150ee5219f 100644 --- a/python/ovs/db/types.py +++ b/python/ovs/db/types.py @@ -45,10 +45,15 @@ class AtomicType(object): def __str__(self): return self.name + def to_lvalue_string(self): + if self == StringType: + return 's' + return self.name + def to_string(self): return self.name - def to_Cvalue_string(self): + def to_rvalue_string(self): if self == StringType: return 's->' + self.name return self.name @@ -382,17 +387,6 @@ class BaseType(object): else: return "%(dst)s = %(src)s;" % args - def assign_c_value_casting_away_const(self, dst, src, refTable=True): - args = {'dst': dst, 'src': src} - if self.ref_table_name: - if not refTable: - return "%(dst)s = *%(src)s;" % args - 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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
