diff mbox series

[ovs-dev] ovsdb-data: Deduplicate string atoms.

Message ID 20210923001353.4072533-1-i.maximets@ovn.org
State Accepted
Headers show
Series [ovs-dev] ovsdb-data: Deduplicate string atoms. | expand

Checks

Context Check Description
ovsrobot/apply-robot fail apply and check: fail

Commit Message

Ilya Maximets Sept. 23, 2021, 12:13 a.m. UTC 
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.

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.

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 <i.maximets@ovn.org>
---

There is a small conflict in lib/db-ctl-base.c with the 'set
optimizations' patch-set, but it can be easily resolved if
someone wants to try these patches together.

 lib/db-ctl-base.c      |  11 +--
 lib/ovsdb-cs.c         |   2 +-
 lib/ovsdb-data.c       |  45 ++++++-----
 lib/ovsdb-data.h       |  29 ++++++-
 lib/ovsdb-idl.c        |   3 +-
 ovsdb/ovsdb-idlc.in    | 179 ++++++++++++++++++++++++++++-------------
 ovsdb/ovsdb-server.c   |   6 +-
 ovsdb/ovsdb-util.c     |  16 ++--
 ovsdb/rbac.c           |   8 +-
 python/ovs/db/data.py  |  43 +++++++---
 python/ovs/db/types.py |   7 +-
 tests/test-ovsdb.c     |  14 ++--
 12 files changed, 244 insertions(+), 119 deletions(-)

Comments

Dumitru Ceara Sept. 23, 2021, 8:51 p.m. UTC | #1 
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 <dceara@redhat.com>

> 
> 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 <i.maximets@ovn.org>
> ---

[...]

> 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
Mark Gray Sept. 24, 2021, 9:12 a.m. UTC | #2 
On 23/09/2021 01:13, 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.
> 
> 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.
> 
> 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.
> 

Great!

> 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 <i.maximets@ovn.org>
> ---
> 
> There is a small conflict in lib/db-ctl-base.c with the 'set
> optimizations' patch-set, but it can be easily resolved if
> someone wants to try these patches together>
>  lib/db-ctl-base.c      |  11 +--
>  lib/ovsdb-cs.c         |   2 +-
>  lib/ovsdb-data.c       |  45 ++++++-----
>  lib/ovsdb-data.h       |  29 ++++++-
>  lib/ovsdb-idl.c        |   3 +-
>  ovsdb/ovsdb-idlc.in    | 179 ++++++++++++++++++++++++++++-------------
>  ovsdb/ovsdb-server.c   |   6 +-
>  ovsdb/ovsdb-util.c     |  16 ++--
>  ovsdb/rbac.c           |   8 +-
>  python/ovs/db/data.py  |  43 +++++++---
>  python/ovs/db/types.py |   7 +-
>  tests/test-ovsdb.c     |  14 ++--
>  12 files changed, 244 insertions(+), 119 deletions(-)
> 
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index 77cc76a9f..713487797 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -247,15 +247,15 @@ record_id_equals(const union ovsdb_atom *name, enum ovsdb_atomic_type type,
>                   const char *record_id)
>  {
>      if (type == OVSDB_TYPE_STRING) {
> -        if (!strcmp(name->string, record_id)) {
> +        if (!strcmp(name->s->string, record_id)) {
>              return true;
>          }
>  
>          struct uuid uuid;
>          size_t len = strlen(record_id);
>          if (len >= 4
> -            && uuid_from_string(&uuid, name->string)
> -            && !strncmp(name->string, record_id, len)) {
> +            && uuid_from_string(&uuid, name->s->string)
> +            && !strncmp(name->s->string, record_id, len)) {
>              return true;
>          }
>  
> @@ -318,11 +318,12 @@ get_row_by_id(struct ctl_context *ctx,
>          if (!id->key) {
>              name = datum->n == 1 ? &datum->keys[0] : NULL;
>          } else {
> -            const union ovsdb_atom key_atom
> -                = { .string = CONST_CAST(char *, id->key) };
> +            union ovsdb_atom key_atom = {
> +                .s = ovsdb_atom_string_create(CONST_CAST(char *, id->key)) };
>              unsigned int i = ovsdb_datum_find_key(datum, &key_atom,
>                                                    OVSDB_TYPE_STRING);
>              name = i == UINT_MAX ? NULL : &datum->values[i];
> +            ovsdb_atom_destroy(&key_atom, OVSDB_TYPE_STRING);
>          }
>          if (!name) {
>              continue;
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index 659d49dbf..fcb6fe1b3 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -1833,7 +1833,7 @@ server_column_get_string(const struct server_row *row,
>  {
>      ovs_assert(server_columns[index].type.key.type == OVSDB_TYPE_STRING);
>      const struct ovsdb_datum *d = &row->data[index];
> -    return d->n == 1 ? d->keys[0].string : default_value;
> +    return d->n == 1 ? d->keys[0].s->string : default_value;
>  }
>  
>  static bool
> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> index 1f491a98b..0ed8f01b6 100644
> --- a/lib/ovsdb-data.c
> +++ b/lib/ovsdb-data.c
> @@ -74,7 +74,7 @@ ovsdb_atom_init_default(union ovsdb_atom *atom, enum ovsdb_atomic_type type)
>          break;
>  
>      case OVSDB_TYPE_STRING:
> -        atom->string = xmemdup("", 1);
> +        atom->s = ovsdb_atom_string_create_nocopy(xmemdup("", 1));
>          break;
>  
>      case OVSDB_TYPE_UUID:
> @@ -136,7 +136,7 @@ ovsdb_atom_is_default(const union ovsdb_atom *atom,
>          return atom->boolean == false;
>  
>      case OVSDB_TYPE_STRING:
> -        return atom->string[0] == '\0';
> +        return atom->s->string[0] == '\0';

perhaps a helper function would allow you to hide some of this from a
client. e.g.

char *ovsdb_atom_string_get(struct ovsdb_atom)
>  
>      case OVSDB_TYPE_UUID:
>          return uuid_is_zero(&atom->uuid);
> @@ -172,7 +172,8 @@ ovsdb_atom_clone(union ovsdb_atom *new, const union ovsdb_atom *old,
>          break;
>  
>      case OVSDB_TYPE_STRING:
> -        new->string = xstrdup(old->string);
> +        new->s = old->s;
> +        new->s->n_refs++;
>          break;
>  
>      case OVSDB_TYPE_UUID:
> @@ -214,7 +215,7 @@ ovsdb_atom_hash(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
>          return hash_boolean(atom->boolean, basis);
>  
>      case OVSDB_TYPE_STRING:
> -        return hash_string(atom->string, basis);
> +        return hash_string(atom->s->string, basis);
>  
>      case OVSDB_TYPE_UUID:
>          return hash_int(uuid_hash(&atom->uuid), basis);
> @@ -246,7 +247,7 @@ ovsdb_atom_compare_3way(const union ovsdb_atom *a,
>          return a->boolean - b->boolean;
>  
>      case OVSDB_TYPE_STRING:
> -        return strcmp(a->string, b->string);
> +        return a->s == b->s ? 0 : strcmp(a->s->string, b->s->string);
>  
>      case OVSDB_TYPE_UUID:
>          return uuid_compare_3way(&a->uuid, &b->uuid);
> @@ -404,7 +405,7 @@ ovsdb_atom_from_json__(union ovsdb_atom *atom,
>  
>      case OVSDB_TYPE_STRING:
>          if (json->type == JSON_STRING) {
> -            atom->string = xstrdup(json->string);
> +            atom->s = ovsdb_atom_string_create(json->string);
>              return NULL;
>          }
>          break;
> @@ -473,7 +474,7 @@ ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type)
>          return json_boolean_create(atom->boolean);
>  
>      case OVSDB_TYPE_STRING:
> -        return json_string_create(atom->string);
> +        return json_string_create(atom->s->string);
>  
>      case OVSDB_TYPE_UUID:
>          return wrap_json("uuid", json_string_create_nocopy(
> @@ -551,14 +552,18 @@ ovsdb_atom_from_string__(union ovsdb_atom *atom,
>              if (s_len < 2 || s[s_len - 1] != '"') {
>                  return xasprintf("%s: missing quote at end of "
>                                   "quoted string", s);
> -            } else if (!json_string_unescape(s + 1, s_len - 2,
> -                                             &atom->string)) {
> -                char *error = xasprintf("%s: %s", s, atom->string);
> -                free(atom->string);
> -                return error;
> +            } else {
> +                char *res;
> +                if (json_string_unescape(s + 1, s_len - 2, &res)) {
> +                    atom->s = ovsdb_atom_string_create_nocopy(res);
> +                } else {
> +                    char *error = xasprintf("%s: %s", s, res);
> +                    free(res);
> +                    return error;
> +                }
>              }
>          } else {
> -            atom->string = xstrdup(s);
> +            atom->s = ovsdb_atom_string_create(s);
>          }
>          break;
>  
> @@ -721,14 +726,14 @@ ovsdb_atom_to_string(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
>          break;
>  
>      case OVSDB_TYPE_STRING:
> -        if (string_needs_quotes(atom->string)) {
> +        if (string_needs_quotes(atom->s->string)) {
>              struct json json;
>  
>              json.type = JSON_STRING;
> -            json.string = atom->string;
> +            json.string = atom->s->string;
>              json_to_ds(&json, 0, out);
>          } else {
> -            ds_put_cstr(out, atom->string);
> +            ds_put_cstr(out, atom->s->string);
>          }
>          break;
>  
> @@ -750,7 +755,7 @@ ovsdb_atom_to_bare(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
>                     struct ds *out)
>  {
>      if (type == OVSDB_TYPE_STRING) {
> -        ds_put_cstr(out, atom->string);
> +        ds_put_cstr(out, atom->s->string);
>      } else {
>          ovsdb_atom_to_string(atom, type, out);
>      }
> @@ -877,7 +882,7 @@ ovsdb_atom_check_constraints(const union ovsdb_atom *atom,
>          return NULL;
>  
>      case OVSDB_TYPE_STRING:
> -        return check_string_constraints(atom->string, &base->string);
> +        return check_string_constraints(atom->s->string, &base->string);
>  
>      case OVSDB_TYPE_UUID:
>          return NULL;
> @@ -1691,8 +1696,8 @@ ovsdb_datum_from_smap(struct ovsdb_datum *datum, const struct smap *smap)
>      struct smap_node *node;
>      size_t i = 0;
>      SMAP_FOR_EACH (node, smap) {
> -        datum->keys[i].string = xstrdup(node->key);
> -        datum->values[i].string = xstrdup(node->value);
> +        datum->keys[i].s = ovsdb_atom_string_create(node->key);
> +        datum->values[i].s = ovsdb_atom_string_create(node->value);
>          i++;
>      }
>      ovs_assert(i == datum->n);
> 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;
> +};
> +
> +static inline struct ovsdb_atom_string *
> +ovsdb_atom_string_create_nocopy(char *str)
> +{
> +    struct ovsdb_atom_string *s = xzalloc(sizeof *s);
> +
> +    s->string = str;
> +    s->n_refs = 1;
> +    return s;
> +}
> +
> +static inline struct ovsdb_atom_string *
> +ovsdb_atom_string_create(const char *str)
> +{
> +    return ovsdb_atom_string_create_nocopy(xstrdup(str));
> +}
> +
>  /* One value of an atomic type (given by enum ovs_atomic_type). */
>  union ovsdb_atom {
>      int64_t integer;
>      double real;
>      bool boolean;
> -    char *string;
> +    struct ovsdb_atom_string *s;
>      struct uuid uuid;
>  };
>  
> @@ -66,8 +88,9 @@ ovsdb_atom_needs_destruction(enum ovsdb_atomic_type type)
>  static inline void
>  ovsdb_atom_destroy(union ovsdb_atom *atom, enum ovsdb_atomic_type type)
>  {
> -    if (type == OVSDB_TYPE_STRING) {
> -        free(atom->string);
> +    if (type == OVSDB_TYPE_STRING && !--atom->s->n_refs) {
> +        free(atom->s->string);
> +        free(atom->s);
>      }
>  }
>  
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index d3caccec8..40125f234 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -1951,8 +1951,7 @@ ovsdb_idl_index_destroy_row(const struct ovsdb_idl_row *row_)
>      BITMAP_FOR_EACH_1 (i, class->n_columns, row->written) {
>          c = &class->columns[i];
>          (c->unparse) (row);
> -        free(row->new_datum[i].values);
> -        free(row->new_datum[i].keys);
> +        ovsdb_datum_destroy(&row->new_datum[i], &c->type);
>      }
>      free(row->new_datum);
>      free(row->written);
> diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
> index f19e92915..877f903cb 100755
> --- a/ovsdb/ovsdb-idlc.in
> +++ b/ovsdb/ovsdb-idlc.in
> @@ -577,20 +577,20 @@ static void
>                  print("    smap_init(&row->%s);" % columnName)
>                  print("    for (size_t i = 0; i < datum->n; i++) {")
>                  print("        smap_add(&row->%s," % columnName)
> -                print("                 datum->keys[i].string,")
> -                print("                 datum->values[i].string);")
> +                print("                 datum->keys[i].s->string,")
> +                print("                 datum->values[i].s->string);")
>                  print("    }")
>              elif (type.n_min == 1 and type.n_max == 1) or type.is_optional_pointer():
>                  print("")
>                  print("    if (datum->n >= 1) {")
>                  if not type.key.ref_table:
> -                    print("        %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_string()))
> +                    print("        %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_Cvalue_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_string()))
> +                        print("        %s = datum->values[0].%s;" % (valueVar, type.value.type.to_Cvalue_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_string()
> +                    keySrc = "datum->keys[i].%s" % type.key.type.to_Cvalue_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_string()
> +                    valueSrc = "datum->values[i].%s" % type.value.type.to_Cvalue_string()
>                  print("        if (!row->n_%s) {" % (columnName))
>  
>                  print("            %s = xmalloc(%s * sizeof *%s);" % (
> @@ -936,45 +936,57 @@ void
>          'args': ', '.join(['%(type)s%(name)s'
>                             % m for m in members])})
>              if type.n_min == 1 and type.n_max == 1:
> -                print("    union ovsdb_atom key;")
> +                print("    union ovsdb_atom *key = xmalloc(sizeof *key);")
>                  if type.value:
> -                    print("    union ovsdb_atom value;")
> +                    print("    union ovsdb_atom *value = xmalloc(sizeof *value);")
>                  print("")
>                  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("    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))
>                  if type.value:
> -                    print("    datum.values = &value;")
> -                    print("    "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar))
> +                    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))
>                  else:
>                      print("    datum.values = NULL;")
> -                txn_write_func = "ovsdb_idl_txn_write_clone"
> +                txn_write_func = "ovsdb_idl_txn_write"
>              elif type.is_optional_pointer():
> -                print("    union ovsdb_atom key;")
>                  print("")
>                  print("    if (%s) {" % keyVar)
> +                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
>                  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("        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("    } else {")
>                  print("        datum.n = 0;")
>                  print("        datum.keys = NULL;")
>                  print("    }")
>                  print("    datum.values = NULL;")
> -                txn_write_func = "ovsdb_idl_txn_write_clone"
> +                txn_write_func = "ovsdb_idl_txn_write"
>              elif type.n_max == 1:
> -                print("    union ovsdb_atom key;")
>                  print("")
>                  print("    if (%s) {" % nVar)
> +                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
>                  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("        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("    } else {")
>                  print("        datum.n = 0;")
>                  print("        datum.keys = NULL;")
>                  print("    }")
>                  print("    datum.values = NULL;")
> -                txn_write_func = "ovsdb_idl_txn_write_clone"
> +                txn_write_func = "ovsdb_idl_txn_write"
>              else:
>                  print("")
>                  print("    datum.n = %s;" % nVar)
> @@ -984,9 +996,15 @@ void
>                  else:
>                      print("    datum.values = NULL;")
>                  print("    for (size_t i = 0; i < %s; i++) {" % nVar)
> -                print("        " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar))
> +                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))
>                  if type.value:
> -                    print("        " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar))
> +                    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("    }")
>                  if type.value:
>                      valueType = type.value.toAtomicType()
> @@ -1022,9 +1040,14 @@ 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})
> -
> -                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_key"))
> -                print("    "+ type.value.copyCValue("datum->values[0].%s" % type.value.type.to_string(), "new_value"))
> +                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('''
>      ovsdb_idl_txn_write_partial_map(&row->header_,
>                                      &%(s)s_col_%(c)s,
> @@ -1048,8 +1071,10 @@ 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})
> -
> -                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_key"))
> +                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('''
>      ovsdb_idl_txn_delete_partial_map(&row->header_,
>                                      &%(s)s_col_%(c)s,
> @@ -1075,8 +1100,10 @@ void
>      datum->values = NULL;
>  ''' % {'s': structName, 'c': columnName,
>          'valtype':column.type.key.to_const_c_type(prefix), 't': tableName})
> -
> -                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_value"))
> +                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('''
>      ovsdb_idl_txn_write_partial_set(&row->header_,
>                                      &%(s)s_col_%(c)s,
> @@ -1100,8 +1127,10 @@ 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})
> -
> -                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_value"))
> +                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('''
>      ovsdb_idl_txn_delete_partial_set(&row->header_,
>                                      &%(s)s_col_%(c)s,
> @@ -1169,37 +1198,49 @@ void
>              print("    struct ovsdb_datum datum;")
>              free = []
>              if type.n_min == 1 and type.n_max == 1:
> -                print("    union ovsdb_atom key;")
> +                print("    union ovsdb_atom *key = xmalloc(sizeof *key);")
>                  if type.value:
> -                    print("    union ovsdb_atom value;")
> +                    print("    union ovsdb_atom *value = xmalloc(sizeof *value);")
>                  print("")
>                  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, refTable=False))
> +                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))
>                  if type.value:
> -                    print("    datum.values = &value;")
> -                    print("    "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar, refTable=False))
> +                    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))
>                  else:
>                      print("    datum.values = NULL;")
>              elif type.is_optional_pointer():
> -                print("    union ovsdb_atom key;")
>                  print("")
>                  print("    if (%s) {" % keyVar)
> +                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
>                  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, refTable=False))
> +                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("    } else {")
>                  print("        datum.n = 0;")
>                  print("        datum.keys = NULL;")
>                  print("    }")
>                  print("    datum.values = NULL;")
>              elif type.n_max == 1:
> -                print("    union ovsdb_atom key;")
>                  print("")
>                  print("    if (%s) {" % nVar)
> +                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
>                  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, refTable=False))
> +                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("    } else {")
>                  print("        datum.n = 0;")
>                  print("        datum.keys = NULL;")
> @@ -1208,16 +1249,20 @@ void
>              else:
>                  print("    datum.n = %s;" % nVar)
>                  print("    datum.keys = %s ? xmalloc(%s * sizeof *datum.keys) : NULL;" % (nVar, nVar))
> -                free += ['datum.keys']
>                  if type.value:
>                      print("    datum.values = xmalloc(%s * sizeof *datum.values);" % nVar)
> -                    free += ['datum.values']
>                  else:
>                      print("    datum.values = NULL;")
>                  print("    for (size_t i = 0; i < %s; i++) {" % nVar)
> -                print("        " + type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar, refTable=False))
> +                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))
>                  if type.value:
> -                    print("        " + type.value.assign_c_value_casting_away_const("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar, refTable=False))
> +                    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("    }")
>                  if type.value:
>                      valueType = type.value.toAtomicType()
> @@ -1237,8 +1282,8 @@ void
>         's': structName,
>         'S': structName.upper(),
>         'c': columnName})
> -            for var in free:
> -                print("    free(%s);" % var)
> +            print("    ovsdb_datum_destroy(&datum, &%(s)s_col_%(c)s.type);" \
> +                  % {'s': structName, 'c': columnName})
>              print("}")
>  
>  # Index table related functions
> @@ -1335,8 +1380,8 @@ struct %(s)s *
>  
>          i = 0;
>          SMAP_FOR_EACH (node, %(c)s) {
> -            datum->keys[i].string = node->key;
> -            datum->values[i].string = node->value;
> +            datum->keys[i].s = ovsdb_atom_string_create(node->key);
> +            datum->values[i].s = ovsdb_atom_string_create(node->value);
>              i++;
>          }
>          ovsdb_datum_sort_unique(datum, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
> @@ -1385,10 +1430,16 @@ struct %(s)s *
>                  print()
>                  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))
> +                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))
>                  if type.value:
>                      print("    datum.values = value;")
> -                    print("    "+ type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar))
> +                    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))
>                  else:
>                      print("    datum.values = NULL;")
>                  txn_write_func = "ovsdb_idl_index_write"
> @@ -1399,7 +1450,10 @@ struct %(s)s *
>                  print("        key = xmalloc(sizeof (union ovsdb_atom));")
>                  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))
> +                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("    } else {")
>                  print("        datum.n = 0;")
>                  print("        datum.keys = NULL;")
> @@ -1413,7 +1467,10 @@ struct %(s)s *
>                  print("        key = xmalloc(sizeof(union ovsdb_atom));")
>                  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))
> +                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("    } else {")
>                  print("        datum.n = 0;")
>                  print("        datum.keys = NULL;")
> @@ -1430,9 +1487,15 @@ struct %(s)s *
>                  else:
>                      print("    datum.values = NULL;")
>                  print("    for (i = 0; i < %s; i++) {" % nVar)
> -                print("        " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar))
> +                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))
>                  if type.value:
> -                    print("        " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar))
> +                    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("    }")
>                  if type.value:
>                      valueType = type.value.toAtomicType()
> diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
> index 0b3d2bb71..b34d97e29 100644
> --- a/ovsdb/ovsdb-server.c
> +++ b/ovsdb/ovsdb-server.c
> @@ -904,8 +904,8 @@ query_db_string(const struct shash *all_dbs, const char *name,
>  
>              datum = &row->fields[column->index];
>              for (i = 0; i < datum->n; i++) {
> -                if (datum->keys[i].string[0]) {
> -                    return datum->keys[i].string;
> +                if (datum->keys[i].s->string[0]) {
> +                    return datum->keys[i].s->string;
>                  }
>              }
>          }
> @@ -1018,7 +1018,7 @@ query_db_remotes(const char *name, const struct shash *all_dbs,
>  
>              datum = &row->fields[column->index];
>              for (i = 0; i < datum->n; i++) {
> -                add_remote(remotes, datum->keys[i].string);
> +                add_remote(remotes, datum->keys[i].s->string);
>              }
>          }
>      } else if (column->type.key.type == OVSDB_TYPE_UUID
> diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
> index c4075cdae..6d7be066b 100644
> --- a/ovsdb/ovsdb-util.c
> +++ b/ovsdb/ovsdb-util.c
> @@ -111,13 +111,13 @@ ovsdb_util_read_map_string_column(const struct ovsdb_row *row,
>  
>      for (i = 0; i < datum->n; i++) {
>          atom_key = &datum->keys[i];
> -        if (!strcmp(atom_key->string, key)) {
> +        if (!strcmp(atom_key->s->string, key)) {
>              atom_value = &datum->values[i];
>              break;
>          }
>      }
>  
> -    return atom_value ? atom_value->string : NULL;
> +    return atom_value ? atom_value->s->string : NULL;
>  }
>  
>  /* Read string-uuid key-values from a map.  Returns the row associated with
> @@ -143,7 +143,7 @@ ovsdb_util_read_map_string_uuid_column(const struct ovsdb_row *row,
>      const struct ovsdb_datum *datum = &row->fields[column->index];
>      for (size_t i = 0; i < datum->n; i++) {
>          union ovsdb_atom *atom_key = &datum->keys[i];
> -        if (!strcmp(atom_key->string, key)) {
> +        if (!strcmp(atom_key->s->string, key)) {
>              const union ovsdb_atom *atom_value = &datum->values[i];
>              return ovsdb_table_get_row(ref_table, &atom_value->uuid);
>          }
> @@ -181,7 +181,7 @@ ovsdb_util_read_string_column(const struct ovsdb_row *row,
>      const union ovsdb_atom *atom;
>  
>      atom = ovsdb_util_read_column(row, column_name, OVSDB_TYPE_STRING);
> -    *stringp = atom ? atom->string : NULL;
> +    *stringp = atom ? atom->s->string : NULL;
>      return atom != NULL;
>  }
>  
> @@ -269,8 +269,10 @@ ovsdb_util_write_string_column(struct ovsdb_row *row, const char *column_name,
>                                 const char *string)
>  {
>      if (string) {
> -        const union ovsdb_atom atom = { .string = CONST_CAST(char *, string) };
> +        union ovsdb_atom atom = {
> +            .s = ovsdb_atom_string_create(CONST_CAST(char *, string)) };
>          ovsdb_util_write_singleton(row, column_name, &atom, OVSDB_TYPE_STRING);
> +        ovsdb_atom_destroy(&atom, OVSDB_TYPE_STRING);
>      } else {
>          ovsdb_util_clear_column(row, column_name);
>      }
> @@ -305,8 +307,8 @@ ovsdb_util_write_string_string_column(struct ovsdb_row *row,
>      datum->values = xmalloc(n * sizeof *datum->values);
>  
>      for (i = 0; i < n; ++i) {
> -        datum->keys[i].string = keys[i];
> -        datum->values[i].string = values[i];
> +        datum->keys[i].s = ovsdb_atom_string_create_nocopy(keys[i]);
> +        datum->values[i].s = ovsdb_atom_string_create_nocopy(values[i]);
>      }
>  
>      /* Sort and check constraints. */
> diff --git a/ovsdb/rbac.c b/ovsdb/rbac.c
> index 2986027c9..ff411675f 100644
> --- a/ovsdb/rbac.c
> +++ b/ovsdb/rbac.c
> @@ -53,8 +53,8 @@ ovsdb_find_row_by_string_key(const struct ovsdb_table *table,
>          HMAP_FOR_EACH (row, hmap_node, &table->rows) {
>              const struct ovsdb_datum *datum = &row->fields[column->index];
>              for (size_t i = 0; i < datum->n; i++) {
> -                if (datum->keys[i].string[0] &&
> -                    !strcmp(key, datum->keys[i].string)) {
> +                if (datum->keys[i].s->string[0] &&
> +                    !strcmp(key, datum->keys[i].s->string)) {
>                      return row;
>                  }
>              }
> @@ -113,7 +113,7 @@ ovsdb_rbac_authorized(const struct ovsdb_row *perms,
>      }
>  
>      for (i = 0; i < datum->n; i++) {
> -        const char *name = datum->keys[i].string;
> +        const char *name = datum->keys[i].s->string;
>          const char *value = NULL;
>          bool is_map;
>  
> @@ -271,7 +271,7 @@ rbac_column_modification_permitted(const struct ovsdb_column *column,
>      size_t i;
>  
>      for (i = 0; i < modifiable->n; i++) {
> -        char *name = modifiable->keys[i].string;
> +        char *name = modifiable->keys[i].s->string;
>  
>          if (!strcmp(name, column->name)) {
>              return true;
> diff --git a/python/ovs/db/data.py b/python/ovs/db/data.py
> index 2a2102d6b..99bf80ed6 100644
> --- a/python/ovs/db/data.py
> +++ b/python/ovs/db/data.py
> @@ -204,7 +204,7 @@ class Atom(object):
>              else:
>                  return '.boolean = false'
>          elif self.type == ovs.db.types.StringType:
> -            return '.string = "%s"' % escapeCString(self.value)
> +            return '.s = %s' % escapeCString(self.value)
>          elif self.type == ovs.db.types.UuidType:
>              return '.uuid = %s' % ovs.ovsuuid.to_c_assignment(self.value)
>  
> @@ -563,16 +563,41 @@ class Datum(object):
>          if n == 0:
>              return ["static struct ovsdb_datum %s = { .n = 0 };"]
>  
> -        s = ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)]
> -        for key in sorted(self.values):
> -            s += ["    { %s }," % key.cInitAtom(key)]
> -        s += ["};"]
> +        s = []
> +        if self.type.key.type == ovs.db.types.StringType:
> +            s += ["static struct ovsdb_atom_string %s_key_strings[%d] = {"
> +                  % (name, n)]
> +            for key in sorted(self.values):
> +                s += ['    { .string = "%s", .n_refs = 2 },'
> +                      % escapeCString(key.value)]
> +            s += ["};"]
> +            s += ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)]
> +            for i in range(n):
> +                s += ["    { .s = &%s_key_strings[%d] }," % (name, i)]
> +            s += ["};"]
> +        else:
> +            s = ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)]
> +            for key in sorted(self.values):
> +                s += ["    { %s }," % key.cInitAtom(key)]
> +            s += ["};"]
>  
>          if self.type.value:
> -            s = ["static union ovsdb_atom %s_values[%d] = {" % (name, n)]
> -            for k, v in sorted(self.values.items()):
> -                s += ["    { %s }," % v.cInitAtom(v)]
> -            s += ["};"]
> +            if self.type.value.type == ovs.db.types.StringType:
> +                s += ["static struct ovsdb_atom_string %s_val_strings[%d] = {"
> +                      % (name, n)]
> +                for k, v in sorted(self.values):
> +                    s += ['    { .string = "%s", .n_refs = 2 },'
> +                          % escapeCString(v.value)]
> +                s += ["};"]
> +                s += ["static union ovsdb_atom %s_values[%d] = {" % (name, n)]
> +                for i in range(n):
> +                    s += ["    { .s = &%s_val_strings[%d] }," % (name, i)]
> +                s += ["};"]
> +            else:
> +                s = ["static union ovsdb_atom %s_values[%d] = {" % (name, n)]
> +                for k, v in sorted(self.values.items()):
> +                    s += ["    { %s }," % v.cInitAtom(v)]
> +                s += ["};"]
>  
>          s += ["static struct ovsdb_datum %s = {" % name]
>          s += ["    .n = %d," % n]
> diff --git a/python/ovs/db/types.py b/python/ovs/db/types.py
> index 626ae8fc4..18485f701 100644
> --- a/python/ovs/db/types.py
> +++ b/python/ovs/db/types.py
> @@ -48,6 +48,11 @@ class AtomicType(object):
>      def to_string(self):
>          return self.name
>  
> +    def to_Cvalue_string(self):
> +        if self == StringType:
> +            return 's->' + self.name
> +        return self.name
> +
>      def to_json(self):
>          return self.name
>  
> @@ -373,7 +378,7 @@ class BaseType(object):
>                  return "%(dst)s = *%(src)s;" % args
>              return ("%(dst)s = %(src)s->header_.uuid;") % args
>          elif self.type == StringType:
> -            return "%(dst)s = xstrdup(%(src)s);" % args
> +            return "%(dst)s = ovsdb_atom_string_create(%(src)s);" % args
>          else:
>              return "%(dst)s = %(src)s;" % args
>  
> diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
> index 02485b136..c3ce4f361 100644
> --- a/tests/test-ovsdb.c
> +++ b/tests/test-ovsdb.c
> @@ -2727,13 +2727,15 @@ print_idl_row_simple2(const struct idltest_simple2 *s, int step)
>      printf("%03d: name=%s smap=[",
>             step, s->name);
>      for (i = 0; i < smap->n; i++) {
> -        printf("[%s : %s]%s", smap->keys[i].string, smap->values[i].string,
> -                i < smap->n-1? ",": "");
> +        printf("[%s : %s]%s",
> +               smap->keys[i].s->string, smap->values[i].s->string,
> +               i < smap->n - 1 ? "," : "");
>      }
>      printf("] imap=[");
>      for (i = 0; i < imap->n; i++) {
> -        printf("[%"PRId64" : %s]%s", imap->keys[i].integer, imap->values[i].string,
> -                i < imap->n-1? ",":"");
> +        printf("[%"PRId64" : %s]%s",
> +               imap->keys[i].integer, imap->values[i].s->string,
> +               i < imap->n - 1 ? "," : "");
>      }
>      printf("]\n");
>  }
> @@ -2802,8 +2804,8 @@ do_idl_partial_update_map_column(struct ovs_cmdl_context *ctx)
>      myTxn = ovsdb_idl_txn_create(idl);
>      smap = idltest_simple2_get_smap(myRow, OVSDB_TYPE_STRING,
>                                      OVSDB_TYPE_STRING);
> -    strcpy(key_to_delete, smap->keys[0].string);
> -    idltest_simple2_update_smap_delkey(myRow, smap->keys[0].string);
> +    ovs_strlcpy(key_to_delete, smap->keys[0].s->string, sizeof key_to_delete);
> +    idltest_simple2_update_smap_delkey(myRow, smap->keys[0].s->string);
>      ovsdb_idl_txn_commit_block(myTxn);
>      ovsdb_idl_txn_destroy(myTxn);
>      ovsdb_idl_get_initial_snapshot(idl);
>
Ilya Maximets Sept. 24, 2021, 11:01 a.m. UTC | #3 
On 9/24/21 11:12, Mark Gray wrote:
> On 23/09/2021 01:13, 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.
>>
>> 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.
>>
>> 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.
>>
> 
> Great!
> 
>> 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 <i.maximets@ovn.org>
>> ---
>>
>> There is a small conflict in lib/db-ctl-base.c with the 'set
>> optimizations' patch-set, but it can be easily resolved if
>> someone wants to try these patches together>
>>  lib/db-ctl-base.c      |  11 +--
>>  lib/ovsdb-cs.c         |   2 +-
>>  lib/ovsdb-data.c       |  45 ++++++-----
>>  lib/ovsdb-data.h       |  29 ++++++-
>>  lib/ovsdb-idl.c        |   3 +-
>>  ovsdb/ovsdb-idlc.in    | 179 ++++++++++++++++++++++++++++-------------
>>  ovsdb/ovsdb-server.c   |   6 +-
>>  ovsdb/ovsdb-util.c     |  16 ++--
>>  ovsdb/rbac.c           |   8 +-
>>  python/ovs/db/data.py  |  43 +++++++---
>>  python/ovs/db/types.py |   7 +-
>>  tests/test-ovsdb.c     |  14 ++--
>>  12 files changed, 244 insertions(+), 119 deletions(-)
>>
>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
>> index 77cc76a9f..713487797 100644
>> --- a/lib/db-ctl-base.c
>> +++ b/lib/db-ctl-base.c
>> @@ -247,15 +247,15 @@ record_id_equals(const union ovsdb_atom *name, enum ovsdb_atomic_type type,
>>                   const char *record_id)
>>  {
>>      if (type == OVSDB_TYPE_STRING) {
>> -        if (!strcmp(name->string, record_id)) {
>> +        if (!strcmp(name->s->string, record_id)) {
>>              return true;
>>          }
>>  
>>          struct uuid uuid;
>>          size_t len = strlen(record_id);
>>          if (len >= 4
>> -            && uuid_from_string(&uuid, name->string)
>> -            && !strncmp(name->string, record_id, len)) {
>> +            && uuid_from_string(&uuid, name->s->string)
>> +            && !strncmp(name->s->string, record_id, len)) {
>>              return true;
>>          }
>>  
>> @@ -318,11 +318,12 @@ get_row_by_id(struct ctl_context *ctx,
>>          if (!id->key) {
>>              name = datum->n == 1 ? &datum->keys[0] : NULL;
>>          } else {
>> -            const union ovsdb_atom key_atom
>> -                = { .string = CONST_CAST(char *, id->key) };
>> +            union ovsdb_atom key_atom = {
>> +                .s = ovsdb_atom_string_create(CONST_CAST(char *, id->key)) };
>>              unsigned int i = ovsdb_datum_find_key(datum, &key_atom,
>>                                                    OVSDB_TYPE_STRING);
>>              name = i == UINT_MAX ? NULL : &datum->values[i];
>> +            ovsdb_atom_destroy(&key_atom, OVSDB_TYPE_STRING);
>>          }
>>          if (!name) {
>>              continue;
>> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
>> index 659d49dbf..fcb6fe1b3 100644
>> --- a/lib/ovsdb-cs.c
>> +++ b/lib/ovsdb-cs.c
>> @@ -1833,7 +1833,7 @@ server_column_get_string(const struct server_row *row,
>>  {
>>      ovs_assert(server_columns[index].type.key.type == OVSDB_TYPE_STRING);
>>      const struct ovsdb_datum *d = &row->data[index];
>> -    return d->n == 1 ? d->keys[0].string : default_value;
>> +    return d->n == 1 ? d->keys[0].s->string : default_value;
>>  }
>>  
>>  static bool
>> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
>> index 1f491a98b..0ed8f01b6 100644
>> --- a/lib/ovsdb-data.c
>> +++ b/lib/ovsdb-data.c
>> @@ -74,7 +74,7 @@ ovsdb_atom_init_default(union ovsdb_atom *atom, enum ovsdb_atomic_type type)
>>          break;
>>  
>>      case OVSDB_TYPE_STRING:
>> -        atom->string = xmemdup("", 1);
>> +        atom->s = ovsdb_atom_string_create_nocopy(xmemdup("", 1));
>>          break;
>>  
>>      case OVSDB_TYPE_UUID:
>> @@ -136,7 +136,7 @@ ovsdb_atom_is_default(const union ovsdb_atom *atom,
>>          return atom->boolean == false;
>>  
>>      case OVSDB_TYPE_STRING:
>> -        return atom->string[0] == '\0';
>> +        return atom->s->string[0] == '\0';
> 
> perhaps a helper function would allow you to hide some of this from a
> client. e.g.
> 
> char *ovsdb_atom_string_get(struct ovsdb_atom)

I thought about this, but we have only a handful of places where it is
used.  Another thing is that I want to replace it with json object later
that will have a json_string() access method.  So, it should be fine for
now to not introduce a method that will be replaced anyway.

Also, it seems like it should not be used at all directly, but should be
accessed only through datum/atom APIs, even though it is used in a few
places directly in current code.  So, I don't feel like making it easy
to use.

What do you think?

Best regards, Ilya Maximets.
Mark Gray Sept. 24, 2021, 1:43 p.m. UTC | #4 
On 24/09/2021 12:01, Ilya Maximets wrote:
> On 9/24/21 11:12, Mark Gray wrote:
>> On 23/09/2021 01:13, 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>
>> Great!
>>
>>> 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 <i.maximets@ovn.org>
>>> ---
>>>
>>> There is a small conflict in lib/db-ctl-base.c with the 'set
>>> optimizations' patch-set, but it can be easily resolved if
>>> someone wants to try these patches together>
>>>  lib/db-ctl-base.c      |  11 +--
>>>  lib/ovsdb-cs.c         |   2 +-
>>>  lib/ovsdb-data.c       |  45 ++++++-----
>>>  lib/ovsdb-data.h       |  29 ++++++-
>>>  lib/ovsdb-idl.c        |   3 +-
>>>  ovsdb/ovsdb-idlc.in    | 179 ++++++++++++++++++++++++++++-------------
>>>  ovsdb/ovsdb-server.c   |   6 +-
>>>  ovsdb/ovsdb-util.c     |  16 ++--
>>>  ovsdb/rbac.c           |   8 +-
>>>  python/ovs/db/data.py  |  43 +++++++---
>>>  python/ovs/db/types.py |   7 +-
>>>  tests/test-ovsdb.c     |  14 ++--
>>>  12 files changed, 244 insertions(+), 119 deletions(-)
>>>
>>> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
>>> index 77cc76a9f..713487797 100644
>>> --- a/lib/db-ctl-base.c
>>> +++ b/lib/db-ctl-base.c
>>> @@ -247,15 +247,15 @@ record_id_equals(const union ovsdb_atom *name, enum ovsdb_atomic_type type,
>>>                   const char *record_id)
>>>  {
>>>      if (type == OVSDB_TYPE_STRING) {
>>> -        if (!strcmp(name->string, record_id)) {
>>> +        if (!strcmp(name->s->string, record_id)) {
>>>              return true;
>>>          }
>>>  
>>>          struct uuid uuid;
>>>          size_t len = strlen(record_id);
>>>          if (len >= 4
>>> -            && uuid_from_string(&uuid, name->string)
>>> -            && !strncmp(name->string, record_id, len)) {
>>> +            && uuid_from_string(&uuid, name->s->string)
>>> +            && !strncmp(name->s->string, record_id, len)) {
>>>              return true;
>>>          }
>>>  
>>> @@ -318,11 +318,12 @@ get_row_by_id(struct ctl_context *ctx,
>>>          if (!id->key) {
>>>              name = datum->n == 1 ? &datum->keys[0] : NULL;
>>>          } else {
>>> -            const union ovsdb_atom key_atom
>>> -                = { .string = CONST_CAST(char *, id->key) };
>>> +            union ovsdb_atom key_atom = {
>>> +                .s = ovsdb_atom_string_create(CONST_CAST(char *, id->key)) };
>>>              unsigned int i = ovsdb_datum_find_key(datum, &key_atom,
>>>                                                    OVSDB_TYPE_STRING);
>>>              name = i == UINT_MAX ? NULL : &datum->values[i];
>>> +            ovsdb_atom_destroy(&key_atom, OVSDB_TYPE_STRING);
>>>          }
>>>          if (!name) {
>>>              continue;
>>> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
>>> index 659d49dbf..fcb6fe1b3 100644
>>> --- a/lib/ovsdb-cs.c
>>> +++ b/lib/ovsdb-cs.c
>>> @@ -1833,7 +1833,7 @@ server_column_get_string(const struct server_row *row,
>>>  {
>>>      ovs_assert(server_columns[index].type.key.type == OVSDB_TYPE_STRING);
>>>      const struct ovsdb_datum *d = &row->data[index];
>>> -    return d->n == 1 ? d->keys[0].string : default_value;
>>> +    return d->n == 1 ? d->keys[0].s->string : default_value;
>>>  }
>>>  
>>>  static bool
>>> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
>>> index 1f491a98b..0ed8f01b6 100644
>>> --- a/lib/ovsdb-data.c
>>> +++ b/lib/ovsdb-data.c
>>> @@ -74,7 +74,7 @@ ovsdb_atom_init_default(union ovsdb_atom *atom, enum ovsdb_atomic_type type)
>>>          break;
>>>  
>>>      case OVSDB_TYPE_STRING:
>>> -        atom->string = xmemdup("", 1);
>>> +        atom->s = ovsdb_atom_string_create_nocopy(xmemdup("", 1));
>>>          break;
>>>  
>>>      case OVSDB_TYPE_UUID:
>>> @@ -136,7 +136,7 @@ ovsdb_atom_is_default(const union ovsdb_atom *atom,
>>>          return atom->boolean == false;
>>>  
>>>      case OVSDB_TYPE_STRING:
>>> -        return atom->string[0] == '\0';
>>> +        return atom->s->string[0] == '\0';
>>
>> perhaps a helper function would allow you to hide some of this from a
>> client. e.g.
>>
>> char *ovsdb_atom_string_get(struct ovsdb_atom)
> 
> I thought about this, but we have only a handful of places where it is
> used.  Another thing is that I want to replace it with json object later
> that will have a json_string() access method.  So, it should be fine for
> now to not introduce a method that will be replaced anyway.
> 
> Also, it seems like it should not be used at all directly, but should be
> accessed only through datum/atom APIs, even though it is used in a few
> places directly in current code.  So, I don't feel like making it easy
> to use.
> 
> What do you think?

Considering your future plan about the json object. This sounds good.
> 
> Best regards, Ilya Maximets.
> 

Acked-by: Mark D. Gray <mark.d.gray@redhat.com>
Ilya Maximets Sept. 24, 2021, 1:58 p.m. UTC | #5 
On 9/23/21 22:51, Dumitru Ceara wrote:
> 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.

That's cool!  Thanks for improving this part!

> 
> I also have a tiny nit in ovsdb-data.h.
> 
> With these addressed:
> 
> Acked-by: Dumitru Ceara <dceara@redhat.com>
Thanks, Mark and Dumitru!

I folded suggested changes in and applied the patch.

Best regards, Ilya Maximets.
0-day Robot Sept. 27, 2021, 6:51 p.m. UTC | #6 
Bleep bloop.  Greetings Ilya Maximets, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch' to see the failed patch
Patch failed at 0001 ovsdb-data: Deduplicate string atoms.
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


Patch skipped due to previous failure.

Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
index 77cc76a9f..713487797 100644
--- a/lib/db-ctl-base.c
+++ b/lib/db-ctl-base.c
@@ -247,15 +247,15 @@  record_id_equals(const union ovsdb_atom *name, enum ovsdb_atomic_type type,
                  const char *record_id)
 {
     if (type == OVSDB_TYPE_STRING) {
-        if (!strcmp(name->string, record_id)) {
+        if (!strcmp(name->s->string, record_id)) {
             return true;
         }
 
         struct uuid uuid;
         size_t len = strlen(record_id);
         if (len >= 4
-            && uuid_from_string(&uuid, name->string)
-            && !strncmp(name->string, record_id, len)) {
+            && uuid_from_string(&uuid, name->s->string)
+            && !strncmp(name->s->string, record_id, len)) {
             return true;
         }
 
@@ -318,11 +318,12 @@  get_row_by_id(struct ctl_context *ctx,
         if (!id->key) {
             name = datum->n == 1 ? &datum->keys[0] : NULL;
         } else {
-            const union ovsdb_atom key_atom
-                = { .string = CONST_CAST(char *, id->key) };
+            union ovsdb_atom key_atom = {
+                .s = ovsdb_atom_string_create(CONST_CAST(char *, id->key)) };
             unsigned int i = ovsdb_datum_find_key(datum, &key_atom,
                                                   OVSDB_TYPE_STRING);
             name = i == UINT_MAX ? NULL : &datum->values[i];
+            ovsdb_atom_destroy(&key_atom, OVSDB_TYPE_STRING);
         }
         if (!name) {
             continue;
diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
index 659d49dbf..fcb6fe1b3 100644
--- a/lib/ovsdb-cs.c
+++ b/lib/ovsdb-cs.c
@@ -1833,7 +1833,7 @@  server_column_get_string(const struct server_row *row,
 {
     ovs_assert(server_columns[index].type.key.type == OVSDB_TYPE_STRING);
     const struct ovsdb_datum *d = &row->data[index];
-    return d->n == 1 ? d->keys[0].string : default_value;
+    return d->n == 1 ? d->keys[0].s->string : default_value;
 }
 
 static bool
diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
index 1f491a98b..0ed8f01b6 100644
--- a/lib/ovsdb-data.c
+++ b/lib/ovsdb-data.c
@@ -74,7 +74,7 @@  ovsdb_atom_init_default(union ovsdb_atom *atom, enum ovsdb_atomic_type type)
         break;
 
     case OVSDB_TYPE_STRING:
-        atom->string = xmemdup("", 1);
+        atom->s = ovsdb_atom_string_create_nocopy(xmemdup("", 1));
         break;
 
     case OVSDB_TYPE_UUID:
@@ -136,7 +136,7 @@  ovsdb_atom_is_default(const union ovsdb_atom *atom,
         return atom->boolean == false;
 
     case OVSDB_TYPE_STRING:
-        return atom->string[0] == '\0';
+        return atom->s->string[0] == '\0';
 
     case OVSDB_TYPE_UUID:
         return uuid_is_zero(&atom->uuid);
@@ -172,7 +172,8 @@  ovsdb_atom_clone(union ovsdb_atom *new, const union ovsdb_atom *old,
         break;
 
     case OVSDB_TYPE_STRING:
-        new->string = xstrdup(old->string);
+        new->s = old->s;
+        new->s->n_refs++;
         break;
 
     case OVSDB_TYPE_UUID:
@@ -214,7 +215,7 @@  ovsdb_atom_hash(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
         return hash_boolean(atom->boolean, basis);
 
     case OVSDB_TYPE_STRING:
-        return hash_string(atom->string, basis);
+        return hash_string(atom->s->string, basis);
 
     case OVSDB_TYPE_UUID:
         return hash_int(uuid_hash(&atom->uuid), basis);
@@ -246,7 +247,7 @@  ovsdb_atom_compare_3way(const union ovsdb_atom *a,
         return a->boolean - b->boolean;
 
     case OVSDB_TYPE_STRING:
-        return strcmp(a->string, b->string);
+        return a->s == b->s ? 0 : strcmp(a->s->string, b->s->string);
 
     case OVSDB_TYPE_UUID:
         return uuid_compare_3way(&a->uuid, &b->uuid);
@@ -404,7 +405,7 @@  ovsdb_atom_from_json__(union ovsdb_atom *atom,
 
     case OVSDB_TYPE_STRING:
         if (json->type == JSON_STRING) {
-            atom->string = xstrdup(json->string);
+            atom->s = ovsdb_atom_string_create(json->string);
             return NULL;
         }
         break;
@@ -473,7 +474,7 @@  ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type)
         return json_boolean_create(atom->boolean);
 
     case OVSDB_TYPE_STRING:
-        return json_string_create(atom->string);
+        return json_string_create(atom->s->string);
 
     case OVSDB_TYPE_UUID:
         return wrap_json("uuid", json_string_create_nocopy(
@@ -551,14 +552,18 @@  ovsdb_atom_from_string__(union ovsdb_atom *atom,
             if (s_len < 2 || s[s_len - 1] != '"') {
                 return xasprintf("%s: missing quote at end of "
                                  "quoted string", s);
-            } else if (!json_string_unescape(s + 1, s_len - 2,
-                                             &atom->string)) {
-                char *error = xasprintf("%s: %s", s, atom->string);
-                free(atom->string);
-                return error;
+            } else {
+                char *res;
+                if (json_string_unescape(s + 1, s_len - 2, &res)) {
+                    atom->s = ovsdb_atom_string_create_nocopy(res);
+                } else {
+                    char *error = xasprintf("%s: %s", s, res);
+                    free(res);
+                    return error;
+                }
             }
         } else {
-            atom->string = xstrdup(s);
+            atom->s = ovsdb_atom_string_create(s);
         }
         break;
 
@@ -721,14 +726,14 @@  ovsdb_atom_to_string(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
         break;
 
     case OVSDB_TYPE_STRING:
-        if (string_needs_quotes(atom->string)) {
+        if (string_needs_quotes(atom->s->string)) {
             struct json json;
 
             json.type = JSON_STRING;
-            json.string = atom->string;
+            json.string = atom->s->string;
             json_to_ds(&json, 0, out);
         } else {
-            ds_put_cstr(out, atom->string);
+            ds_put_cstr(out, atom->s->string);
         }
         break;
 
@@ -750,7 +755,7 @@  ovsdb_atom_to_bare(const union ovsdb_atom *atom, enum ovsdb_atomic_type type,
                    struct ds *out)
 {
     if (type == OVSDB_TYPE_STRING) {
-        ds_put_cstr(out, atom->string);
+        ds_put_cstr(out, atom->s->string);
     } else {
         ovsdb_atom_to_string(atom, type, out);
     }
@@ -877,7 +882,7 @@  ovsdb_atom_check_constraints(const union ovsdb_atom *atom,
         return NULL;
 
     case OVSDB_TYPE_STRING:
-        return check_string_constraints(atom->string, &base->string);
+        return check_string_constraints(atom->s->string, &base->string);
 
     case OVSDB_TYPE_UUID:
         return NULL;
@@ -1691,8 +1696,8 @@  ovsdb_datum_from_smap(struct ovsdb_datum *datum, const struct smap *smap)
     struct smap_node *node;
     size_t i = 0;
     SMAP_FOR_EACH (node, smap) {
-        datum->keys[i].string = xstrdup(node->key);
-        datum->values[i].string = xstrdup(node->value);
+        datum->keys[i].s = ovsdb_atom_string_create(node->key);
+        datum->values[i].s = ovsdb_atom_string_create(node->value);
         i++;
     }
     ovs_assert(i == datum->n);
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;
+};
+
+static inline struct ovsdb_atom_string *
+ovsdb_atom_string_create_nocopy(char *str)
+{
+    struct ovsdb_atom_string *s = xzalloc(sizeof *s);
+
+    s->string = str;
+    s->n_refs = 1;
+    return s;
+}
+
+static inline struct ovsdb_atom_string *
+ovsdb_atom_string_create(const char *str)
+{
+    return ovsdb_atom_string_create_nocopy(xstrdup(str));
+}
+
 /* One value of an atomic type (given by enum ovs_atomic_type). */
 union ovsdb_atom {
     int64_t integer;
     double real;
     bool boolean;
-    char *string;
+    struct ovsdb_atom_string *s;
     struct uuid uuid;
 };
 
@@ -66,8 +88,9 @@  ovsdb_atom_needs_destruction(enum ovsdb_atomic_type type)
 static inline void
 ovsdb_atom_destroy(union ovsdb_atom *atom, enum ovsdb_atomic_type type)
 {
-    if (type == OVSDB_TYPE_STRING) {
-        free(atom->string);
+    if (type == OVSDB_TYPE_STRING && !--atom->s->n_refs) {
+        free(atom->s->string);
+        free(atom->s);
     }
 }
 
diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index d3caccec8..40125f234 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -1951,8 +1951,7 @@  ovsdb_idl_index_destroy_row(const struct ovsdb_idl_row *row_)
     BITMAP_FOR_EACH_1 (i, class->n_columns, row->written) {
         c = &class->columns[i];
         (c->unparse) (row);
-        free(row->new_datum[i].values);
-        free(row->new_datum[i].keys);
+        ovsdb_datum_destroy(&row->new_datum[i], &c->type);
     }
     free(row->new_datum);
     free(row->written);
diff --git a/ovsdb/ovsdb-idlc.in b/ovsdb/ovsdb-idlc.in
index f19e92915..877f903cb 100755
--- a/ovsdb/ovsdb-idlc.in
+++ b/ovsdb/ovsdb-idlc.in
@@ -577,20 +577,20 @@  static void
                 print("    smap_init(&row->%s);" % columnName)
                 print("    for (size_t i = 0; i < datum->n; i++) {")
                 print("        smap_add(&row->%s," % columnName)
-                print("                 datum->keys[i].string,")
-                print("                 datum->values[i].string);")
+                print("                 datum->keys[i].s->string,")
+                print("                 datum->values[i].s->string);")
                 print("    }")
             elif (type.n_min == 1 and type.n_max == 1) or type.is_optional_pointer():
                 print("")
                 print("    if (datum->n >= 1) {")
                 if not type.key.ref_table:
-                    print("        %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_string()))
+                    print("        %s = datum->keys[0].%s;" % (keyVar, type.key.type.to_Cvalue_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_string()))
+                        print("        %s = datum->values[0].%s;" % (valueVar, type.value.type.to_Cvalue_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_string()
+                    keySrc = "datum->keys[i].%s" % type.key.type.to_Cvalue_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_string()
+                    valueSrc = "datum->values[i].%s" % type.value.type.to_Cvalue_string()
                 print("        if (!row->n_%s) {" % (columnName))
 
                 print("            %s = xmalloc(%s * sizeof *%s);" % (
@@ -936,45 +936,57 @@  void
         'args': ', '.join(['%(type)s%(name)s'
                            % m for m in members])})
             if type.n_min == 1 and type.n_max == 1:
-                print("    union ovsdb_atom key;")
+                print("    union ovsdb_atom *key = xmalloc(sizeof *key);")
                 if type.value:
-                    print("    union ovsdb_atom value;")
+                    print("    union ovsdb_atom *value = xmalloc(sizeof *value);")
                 print("")
                 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("    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))
                 if type.value:
-                    print("    datum.values = &value;")
-                    print("    "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar))
+                    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))
                 else:
                     print("    datum.values = NULL;")
-                txn_write_func = "ovsdb_idl_txn_write_clone"
+                txn_write_func = "ovsdb_idl_txn_write"
             elif type.is_optional_pointer():
-                print("    union ovsdb_atom key;")
                 print("")
                 print("    if (%s) {" % keyVar)
+                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
                 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("        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("    } else {")
                 print("        datum.n = 0;")
                 print("        datum.keys = NULL;")
                 print("    }")
                 print("    datum.values = NULL;")
-                txn_write_func = "ovsdb_idl_txn_write_clone"
+                txn_write_func = "ovsdb_idl_txn_write"
             elif type.n_max == 1:
-                print("    union ovsdb_atom key;")
                 print("")
                 print("    if (%s) {" % nVar)
+                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
                 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("        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("    } else {")
                 print("        datum.n = 0;")
                 print("        datum.keys = NULL;")
                 print("    }")
                 print("    datum.values = NULL;")
-                txn_write_func = "ovsdb_idl_txn_write_clone"
+                txn_write_func = "ovsdb_idl_txn_write"
             else:
                 print("")
                 print("    datum.n = %s;" % nVar)
@@ -984,9 +996,15 @@  void
                 else:
                     print("    datum.values = NULL;")
                 print("    for (size_t i = 0; i < %s; i++) {" % nVar)
-                print("        " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar))
+                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))
                 if type.value:
-                    print("        " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar))
+                    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("    }")
                 if type.value:
                     valueType = type.value.toAtomicType()
@@ -1022,9 +1040,14 @@  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})
-
-                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_key"))
-                print("    "+ type.value.copyCValue("datum->values[0].%s" % type.value.type.to_string(), "new_value"))
+                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('''
     ovsdb_idl_txn_write_partial_map(&row->header_,
                                     &%(s)s_col_%(c)s,
@@ -1048,8 +1071,10 @@  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})
-
-                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_key"))
+                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('''
     ovsdb_idl_txn_delete_partial_map(&row->header_,
                                     &%(s)s_col_%(c)s,
@@ -1075,8 +1100,10 @@  void
     datum->values = NULL;
 ''' % {'s': structName, 'c': columnName,
         'valtype':column.type.key.to_const_c_type(prefix), 't': tableName})
-
-                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "new_value"))
+                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('''
     ovsdb_idl_txn_write_partial_set(&row->header_,
                                     &%(s)s_col_%(c)s,
@@ -1100,8 +1127,10 @@  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})
-
-                print("    "+ type.key.copyCValue("datum->keys[0].%s" % type.key.type.to_string(), "delete_value"))
+                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('''
     ovsdb_idl_txn_delete_partial_set(&row->header_,
                                     &%(s)s_col_%(c)s,
@@ -1169,37 +1198,49 @@  void
             print("    struct ovsdb_datum datum;")
             free = []
             if type.n_min == 1 and type.n_max == 1:
-                print("    union ovsdb_atom key;")
+                print("    union ovsdb_atom *key = xmalloc(sizeof *key);")
                 if type.value:
-                    print("    union ovsdb_atom value;")
+                    print("    union ovsdb_atom *value = xmalloc(sizeof *value);")
                 print("")
                 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, refTable=False))
+                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))
                 if type.value:
-                    print("    datum.values = &value;")
-                    print("    "+ type.value.assign_c_value_casting_away_const("value.%s" % type.value.type.to_string(), valueVar, refTable=False))
+                    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))
                 else:
                     print("    datum.values = NULL;")
             elif type.is_optional_pointer():
-                print("    union ovsdb_atom key;")
                 print("")
                 print("    if (%s) {" % keyVar)
+                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
                 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, refTable=False))
+                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("    } else {")
                 print("        datum.n = 0;")
                 print("        datum.keys = NULL;")
                 print("    }")
                 print("    datum.values = NULL;")
             elif type.n_max == 1:
-                print("    union ovsdb_atom key;")
                 print("")
                 print("    if (%s) {" % nVar)
+                print("        union ovsdb_atom *key = xmalloc(sizeof *key);")
                 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, refTable=False))
+                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("    } else {")
                 print("        datum.n = 0;")
                 print("        datum.keys = NULL;")
@@ -1208,16 +1249,20 @@  void
             else:
                 print("    datum.n = %s;" % nVar)
                 print("    datum.keys = %s ? xmalloc(%s * sizeof *datum.keys) : NULL;" % (nVar, nVar))
-                free += ['datum.keys']
                 if type.value:
                     print("    datum.values = xmalloc(%s * sizeof *datum.values);" % nVar)
-                    free += ['datum.values']
                 else:
                     print("    datum.values = NULL;")
                 print("    for (size_t i = 0; i < %s; i++) {" % nVar)
-                print("        " + type.key.assign_c_value_casting_away_const("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar, refTable=False))
+                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))
                 if type.value:
-                    print("        " + type.value.assign_c_value_casting_away_const("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar, refTable=False))
+                    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("    }")
                 if type.value:
                     valueType = type.value.toAtomicType()
@@ -1237,8 +1282,8 @@  void
        's': structName,
        'S': structName.upper(),
        'c': columnName})
-            for var in free:
-                print("    free(%s);" % var)
+            print("    ovsdb_datum_destroy(&datum, &%(s)s_col_%(c)s.type);" \
+                  % {'s': structName, 'c': columnName})
             print("}")
 
 # Index table related functions
@@ -1335,8 +1380,8 @@  struct %(s)s *
 
         i = 0;
         SMAP_FOR_EACH (node, %(c)s) {
-            datum->keys[i].string = node->key;
-            datum->values[i].string = node->value;
+            datum->keys[i].s = ovsdb_atom_string_create(node->key);
+            datum->values[i].s = ovsdb_atom_string_create(node->value);
             i++;
         }
         ovsdb_datum_sort_unique(datum, OVSDB_TYPE_STRING, OVSDB_TYPE_STRING);
@@ -1385,10 +1430,16 @@  struct %(s)s *
                 print()
                 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))
+                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))
                 if type.value:
                     print("    datum.values = value;")
-                    print("    "+ type.value.assign_c_value_casting_away_const("value->%s" % type.value.type.to_string(), valueVar))
+                    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))
                 else:
                     print("    datum.values = NULL;")
                 txn_write_func = "ovsdb_idl_index_write"
@@ -1399,7 +1450,10 @@  struct %(s)s *
                 print("        key = xmalloc(sizeof (union ovsdb_atom));")
                 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))
+                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("    } else {")
                 print("        datum.n = 0;")
                 print("        datum.keys = NULL;")
@@ -1413,7 +1467,10 @@  struct %(s)s *
                 print("        key = xmalloc(sizeof(union ovsdb_atom));")
                 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))
+                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("    } else {")
                 print("        datum.n = 0;")
                 print("        datum.keys = NULL;")
@@ -1430,9 +1487,15 @@  struct %(s)s *
                 else:
                     print("    datum.values = NULL;")
                 print("    for (i = 0; i < %s; i++) {" % nVar)
-                print("        " + type.key.copyCValue("datum.keys[i].%s" % type.key.type.to_string(), "%s[i]" % keyVar))
+                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))
                 if type.value:
-                    print("        " + type.value.copyCValue("datum.values[i].%s" % type.value.type.to_string(), "%s[i]" % valueVar))
+                    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("    }")
                 if type.value:
                     valueType = type.value.toAtomicType()
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 0b3d2bb71..b34d97e29 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -904,8 +904,8 @@  query_db_string(const struct shash *all_dbs, const char *name,
 
             datum = &row->fields[column->index];
             for (i = 0; i < datum->n; i++) {
-                if (datum->keys[i].string[0]) {
-                    return datum->keys[i].string;
+                if (datum->keys[i].s->string[0]) {
+                    return datum->keys[i].s->string;
                 }
             }
         }
@@ -1018,7 +1018,7 @@  query_db_remotes(const char *name, const struct shash *all_dbs,
 
             datum = &row->fields[column->index];
             for (i = 0; i < datum->n; i++) {
-                add_remote(remotes, datum->keys[i].string);
+                add_remote(remotes, datum->keys[i].s->string);
             }
         }
     } else if (column->type.key.type == OVSDB_TYPE_UUID
diff --git a/ovsdb/ovsdb-util.c b/ovsdb/ovsdb-util.c
index c4075cdae..6d7be066b 100644
--- a/ovsdb/ovsdb-util.c
+++ b/ovsdb/ovsdb-util.c
@@ -111,13 +111,13 @@  ovsdb_util_read_map_string_column(const struct ovsdb_row *row,
 
     for (i = 0; i < datum->n; i++) {
         atom_key = &datum->keys[i];
-        if (!strcmp(atom_key->string, key)) {
+        if (!strcmp(atom_key->s->string, key)) {
             atom_value = &datum->values[i];
             break;
         }
     }
 
-    return atom_value ? atom_value->string : NULL;
+    return atom_value ? atom_value->s->string : NULL;
 }
 
 /* Read string-uuid key-values from a map.  Returns the row associated with
@@ -143,7 +143,7 @@  ovsdb_util_read_map_string_uuid_column(const struct ovsdb_row *row,
     const struct ovsdb_datum *datum = &row->fields[column->index];
     for (size_t i = 0; i < datum->n; i++) {
         union ovsdb_atom *atom_key = &datum->keys[i];
-        if (!strcmp(atom_key->string, key)) {
+        if (!strcmp(atom_key->s->string, key)) {
             const union ovsdb_atom *atom_value = &datum->values[i];
             return ovsdb_table_get_row(ref_table, &atom_value->uuid);
         }
@@ -181,7 +181,7 @@  ovsdb_util_read_string_column(const struct ovsdb_row *row,
     const union ovsdb_atom *atom;
 
     atom = ovsdb_util_read_column(row, column_name, OVSDB_TYPE_STRING);
-    *stringp = atom ? atom->string : NULL;
+    *stringp = atom ? atom->s->string : NULL;
     return atom != NULL;
 }
 
@@ -269,8 +269,10 @@  ovsdb_util_write_string_column(struct ovsdb_row *row, const char *column_name,
                                const char *string)
 {
     if (string) {
-        const union ovsdb_atom atom = { .string = CONST_CAST(char *, string) };
+        union ovsdb_atom atom = {
+            .s = ovsdb_atom_string_create(CONST_CAST(char *, string)) };
         ovsdb_util_write_singleton(row, column_name, &atom, OVSDB_TYPE_STRING);
+        ovsdb_atom_destroy(&atom, OVSDB_TYPE_STRING);
     } else {
         ovsdb_util_clear_column(row, column_name);
     }
@@ -305,8 +307,8 @@  ovsdb_util_write_string_string_column(struct ovsdb_row *row,
     datum->values = xmalloc(n * sizeof *datum->values);
 
     for (i = 0; i < n; ++i) {
-        datum->keys[i].string = keys[i];
-        datum->values[i].string = values[i];
+        datum->keys[i].s = ovsdb_atom_string_create_nocopy(keys[i]);
+        datum->values[i].s = ovsdb_atom_string_create_nocopy(values[i]);
     }
 
     /* Sort and check constraints. */
diff --git a/ovsdb/rbac.c b/ovsdb/rbac.c
index 2986027c9..ff411675f 100644
--- a/ovsdb/rbac.c
+++ b/ovsdb/rbac.c
@@ -53,8 +53,8 @@  ovsdb_find_row_by_string_key(const struct ovsdb_table *table,
         HMAP_FOR_EACH (row, hmap_node, &table->rows) {
             const struct ovsdb_datum *datum = &row->fields[column->index];
             for (size_t i = 0; i < datum->n; i++) {
-                if (datum->keys[i].string[0] &&
-                    !strcmp(key, datum->keys[i].string)) {
+                if (datum->keys[i].s->string[0] &&
+                    !strcmp(key, datum->keys[i].s->string)) {
                     return row;
                 }
             }
@@ -113,7 +113,7 @@  ovsdb_rbac_authorized(const struct ovsdb_row *perms,
     }
 
     for (i = 0; i < datum->n; i++) {
-        const char *name = datum->keys[i].string;
+        const char *name = datum->keys[i].s->string;
         const char *value = NULL;
         bool is_map;
 
@@ -271,7 +271,7 @@  rbac_column_modification_permitted(const struct ovsdb_column *column,
     size_t i;
 
     for (i = 0; i < modifiable->n; i++) {
-        char *name = modifiable->keys[i].string;
+        char *name = modifiable->keys[i].s->string;
 
         if (!strcmp(name, column->name)) {
             return true;
diff --git a/python/ovs/db/data.py b/python/ovs/db/data.py
index 2a2102d6b..99bf80ed6 100644
--- a/python/ovs/db/data.py
+++ b/python/ovs/db/data.py
@@ -204,7 +204,7 @@  class Atom(object):
             else:
                 return '.boolean = false'
         elif self.type == ovs.db.types.StringType:
-            return '.string = "%s"' % escapeCString(self.value)
+            return '.s = %s' % escapeCString(self.value)
         elif self.type == ovs.db.types.UuidType:
             return '.uuid = %s' % ovs.ovsuuid.to_c_assignment(self.value)
 
@@ -563,16 +563,41 @@  class Datum(object):
         if n == 0:
             return ["static struct ovsdb_datum %s = { .n = 0 };"]
 
-        s = ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)]
-        for key in sorted(self.values):
-            s += ["    { %s }," % key.cInitAtom(key)]
-        s += ["};"]
+        s = []
+        if self.type.key.type == ovs.db.types.StringType:
+            s += ["static struct ovsdb_atom_string %s_key_strings[%d] = {"
+                  % (name, n)]
+            for key in sorted(self.values):
+                s += ['    { .string = "%s", .n_refs = 2 },'
+                      % escapeCString(key.value)]
+            s += ["};"]
+            s += ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)]
+            for i in range(n):
+                s += ["    { .s = &%s_key_strings[%d] }," % (name, i)]
+            s += ["};"]
+        else:
+            s = ["static union ovsdb_atom %s_keys[%d] = {" % (name, n)]
+            for key in sorted(self.values):
+                s += ["    { %s }," % key.cInitAtom(key)]
+            s += ["};"]
 
         if self.type.value:
-            s = ["static union ovsdb_atom %s_values[%d] = {" % (name, n)]
-            for k, v in sorted(self.values.items()):
-                s += ["    { %s }," % v.cInitAtom(v)]
-            s += ["};"]
+            if self.type.value.type == ovs.db.types.StringType:
+                s += ["static struct ovsdb_atom_string %s_val_strings[%d] = {"
+                      % (name, n)]
+                for k, v in sorted(self.values):
+                    s += ['    { .string = "%s", .n_refs = 2 },'
+                          % escapeCString(v.value)]
+                s += ["};"]
+                s += ["static union ovsdb_atom %s_values[%d] = {" % (name, n)]
+                for i in range(n):
+                    s += ["    { .s = &%s_val_strings[%d] }," % (name, i)]
+                s += ["};"]
+            else:
+                s = ["static union ovsdb_atom %s_values[%d] = {" % (name, n)]
+                for k, v in sorted(self.values.items()):
+                    s += ["    { %s }," % v.cInitAtom(v)]
+                s += ["};"]
 
         s += ["static struct ovsdb_datum %s = {" % name]
         s += ["    .n = %d," % n]
diff --git a/python/ovs/db/types.py b/python/ovs/db/types.py
index 626ae8fc4..18485f701 100644
--- a/python/ovs/db/types.py
+++ b/python/ovs/db/types.py
@@ -48,6 +48,11 @@  class AtomicType(object):
     def to_string(self):
         return self.name
 
+    def to_Cvalue_string(self):
+        if self == StringType:
+            return 's->' + self.name
+        return self.name
+
     def to_json(self):
         return self.name
 
@@ -373,7 +378,7 @@  class BaseType(object):
                 return "%(dst)s = *%(src)s;" % args
             return ("%(dst)s = %(src)s->header_.uuid;") % args
         elif self.type == StringType:
-            return "%(dst)s = xstrdup(%(src)s);" % args
+            return "%(dst)s = ovsdb_atom_string_create(%(src)s);" % args
         else:
             return "%(dst)s = %(src)s;" % args
 
diff --git a/tests/test-ovsdb.c b/tests/test-ovsdb.c
index 02485b136..c3ce4f361 100644
--- a/tests/test-ovsdb.c
+++ b/tests/test-ovsdb.c
@@ -2727,13 +2727,15 @@  print_idl_row_simple2(const struct idltest_simple2 *s, int step)
     printf("%03d: name=%s smap=[",
            step, s->name);
     for (i = 0; i < smap->n; i++) {
-        printf("[%s : %s]%s", smap->keys[i].string, smap->values[i].string,
-                i < smap->n-1? ",": "");
+        printf("[%s : %s]%s",
+               smap->keys[i].s->string, smap->values[i].s->string,
+               i < smap->n - 1 ? "," : "");
     }
     printf("] imap=[");
     for (i = 0; i < imap->n; i++) {
-        printf("[%"PRId64" : %s]%s", imap->keys[i].integer, imap->values[i].string,
-                i < imap->n-1? ",":"");
+        printf("[%"PRId64" : %s]%s",
+               imap->keys[i].integer, imap->values[i].s->string,
+               i < imap->n - 1 ? "," : "");
     }
     printf("]\n");
 }
@@ -2802,8 +2804,8 @@  do_idl_partial_update_map_column(struct ovs_cmdl_context *ctx)
     myTxn = ovsdb_idl_txn_create(idl);
     smap = idltest_simple2_get_smap(myRow, OVSDB_TYPE_STRING,
                                     OVSDB_TYPE_STRING);
-    strcpy(key_to_delete, smap->keys[0].string);
-    idltest_simple2_update_smap_delkey(myRow, smap->keys[0].string);
+    ovs_strlcpy(key_to_delete, smap->keys[0].s->string, sizeof key_to_delete);
+    idltest_simple2_update_smap_delkey(myRow, smap->keys[0].s->string);
     ovsdb_idl_txn_commit_block(myTxn);
     ovsdb_idl_txn_destroy(myTxn);
     ovsdb_idl_get_initial_snapshot(idl);