Message ID | 20240701110257.1052377-4-whitecrowbar@gmail.com |
---|---|
State | Under Review |
Delegated to: | Ilya Maximets |
Headers | show |
Series | Optimize json serialization 2.3 times | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | fail | test: fail |
On 1 Jul 2024, at 13:02, Grigorii Nazarov wrote: Hi Grigorii, This is not an in-depth review, but find some comments below. Cheers, Eelco > Currently serialization is performed by first converting > the internal data representation into JSON objects, followed by > serializing these objects by jsonrpc. This process results in lots of > allocations for these intermediate objects. Consequently, this > not only increases peak memory consumption, but also > demands significantly more CPU work. > > By forming row-update JSONs directly in `ds`, which is then used > to create 'serialized object' JSONs, the overall speed increased > by a factor of 2.3. > > A local benchmark was run on a proprietary Southbound backup. > Both versions, before and after applying the patch, were measured. > For each version, there were two runs with 10 parallel clients, > and two runs with 30 parallel clients. CPU time was recorded > after startup (before clients started running) and after > all clients received all updates. Clients were essentially running > `ovsdb-client monitor-cond unix:pipe OVN_Southbound > '[[\"actions\",\"!=\",\"sdfdfsd\"]]' Logical_Flow`. > Similar results were obtained with other requests > that required a significant amount of data transfer. > The backup size is about 600 MB. Results are measured in seconds. > > Before After > Baseline x10: 9.53 108.54 > Baseline x10: 9.62 108.67 > Baseline x30: 9.69 307.04 > Baseline x30: 9.65 303.32 > Patch x10: 9.67 52.57 > Patch x10: 9.57 53.12 > Patch x30: 9.53 136.33 > Patch x30: 9.63 135.88 Are before and after times reversed? I assume a shorter time is better? Also, how does this relate to the 2.3x speed improvement? > Signed-off-by: Grigorii Nazarov <whitecrowbar@gmail.com> > --- > v2: updated title > v3: fixed bracing > v4: changed patch number from 4/4 to 3/3 > > include/openvswitch/json.h | 1 + > lib/json.c | 8 ++ > lib/ovsdb-data.c | 105 +++++++++++++++++++ > lib/ovsdb-data.h | 3 + > ovsdb/monitor.c | 210 ++++++++++++++++++++++++------------- > 5 files changed, 254 insertions(+), 73 deletions(-) > > diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h > index 555440760..80b9479c7 100644 > --- a/include/openvswitch/json.h > +++ b/include/openvswitch/json.h > @@ -81,6 +81,7 @@ struct json *json_boolean_create(bool); > struct json *json_string_create(const char *); > struct json *json_string_create_nocopy(char *); > struct json *json_serialized_object_create(const struct json *); > +struct json *json_serialized_object_create_from_string(const char *); > struct json *json_integer_create(long long int); > struct json *json_real_create(double); > > diff --git a/lib/json.c b/lib/json.c > index d40e93857..66b1f571f 100644 > --- a/lib/json.c > +++ b/lib/json.c > @@ -199,6 +199,14 @@ json_serialized_object_create(const struct json *src) > return json; > } > > +struct json * > +json_serialized_object_create_from_string(const char *s) > +{ > + struct json *json = json_create(JSON_SERIALIZED_OBJECT); > + json->string = xstrdup(s); > + return json; > +} > + > struct json * > json_serialized_object_create_with_yield(const struct json *src) > { > diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c > index defb048d7..f32b7975a 100644 > --- a/lib/ovsdb-data.c > +++ b/lib/ovsdb-data.c > @@ -492,6 +492,45 @@ ovsdb_atom_to_json__(const union ovsdb_atom *atom, enum ovsdb_atomic_type type, > } > } > > +/* Serializes 'atom', of the specified 'type', into 'ds' in JSON format. > + * > + * Refer to RFC 7047 for the format of the JSON that this function produces. */ > +static void > +ovsdb_atom_to_json_ds(const union ovsdb_atom *atom, > + enum ovsdb_atomic_type type, struct ds *ds) Isn’t this a duplicate of ovsdb_atom_to_string()? > +{ > + switch (type) { > + case OVSDB_TYPE_VOID: > + OVS_NOT_REACHED(); > + > + case OVSDB_TYPE_INTEGER: > + ds_put_format(ds, "%lld", (long long) atom->integer); Rather than typecast this one, just use: ds_put_format(ds, "%"PRId64, atom->integer); > + return; > + > + case OVSDB_TYPE_REAL: > + ds_put_format(ds, "%.*g", DBL_DIG, atom->real); > + return; > + > + case OVSDB_TYPE_BOOLEAN: > + ds_put_cstr(ds, atom->boolean ? "true" : "false"); > + return; > + > + case OVSDB_TYPE_STRING: > + json_to_ds(atom->s, JSSF_SORT, ds); > + return; > + > + case OVSDB_TYPE_UUID: > + ds_put_cstr(ds, "[\"uuid\",\""); > + ds_put_format(ds, UUID_FMT, UUID_ARGS(&atom->uuid)); > + ds_put_cstr(ds, "\"]"); > + return; > + > + case OVSDB_N_TYPES: > + default: > + OVS_NOT_REACHED(); > + } > +} > + > struct json * > ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type) > { > @@ -1445,6 +1484,23 @@ ovsdb_base_to_json(const union ovsdb_atom *atom, > } > } > > +static void > +ovsdb_base_to_json_ds(const union ovsdb_atom *atom, > + const struct ovsdb_base_type *base, > + bool use_row_names, > + struct ds *ds) > +{ Not sure what all this logic is trying to do, as it’s not straightforward. If I reverse the logic it could be the following: if (base->type == OVSDB_TYPE_UUID) { } else { } But I’m not sure this is what you want. > + if (!use_row_names > + || base->type != OVSDB_TYPE_UUID > + || !base->uuid.refTableName) { > + ovsdb_atom_to_json_ds(atom, base->type, ds); > + } else { > + ds_put_cstr(ds, "[\"named-uuid\",\""); > + ds_put_format(ds, UUID_ROW_FMT, UUID_ARGS(&atom->uuid)); > + ds_put_cstr(ds, "\"]"); > + } > +} > + > static struct json * > ovsdb_datum_to_json__(const struct ovsdb_datum *datum, > const struct ovsdb_type *type, > @@ -1482,6 +1538,47 @@ ovsdb_datum_to_json__(const struct ovsdb_datum *datum, > } > } > > +static void > +ovsdb_datum_to_json_ds__(const struct ovsdb_datum *datum, > + const struct ovsdb_type *type, > + bool use_row_names, > + struct ds *ds) > +{ > + if (ovsdb_type_is_map(type)) { > + size_t i; > + > + ds_put_cstr(ds, "[\"map\",["); > + for (i = 0; i < datum->n; i++) { > + if (i != 0) { > + ds_put_char(ds, ','); > + } > + ds_put_char(ds, '['); > + ovsdb_base_to_json_ds(&datum->keys[i], &type->key, > + use_row_names, ds); > + ds_put_char(ds, ','); > + ovsdb_base_to_json_ds(&datum->values[i], &type->value, > + use_row_names, ds); > + ds_put_char(ds, ']'); > + } > + ds_put_cstr(ds, "]]"); > + } else if (datum->n == 1) { > + ovsdb_base_to_json_ds(&datum->keys[0], &type->key, > + use_row_names, ds); > + } else { > + size_t i; > + > + ds_put_cstr(ds, "[\"set\",["); > + for (i = 0; i < datum->n; i++) { > + if (i != 0) { > + ds_put_char(ds, ','); > + } > + ovsdb_base_to_json_ds(&datum->keys[i], &type->key, > + use_row_names, ds); > + } > + ds_put_cstr(ds, "]]"); > + } > +} > + > /* Converts 'datum', of the specified 'type', to JSON format, and returns the > * JSON. The caller is responsible for freeing the returned JSON. > * > @@ -1509,6 +1606,14 @@ ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *datum, > return ovsdb_datum_to_json__(datum, type, true, true); > } > > +void > +ovsdb_datum_to_json_ds(const struct ovsdb_datum *datum, > + const struct ovsdb_type *type, > + struct ds *ds) > +{ > + ovsdb_datum_to_json_ds__(datum, type, false, ds); > +} > + > static const char * > skip_spaces(const char *p) > { > diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h > index c0408ee49..5bc47f0a1 100644 > --- a/lib/ovsdb-data.h > +++ b/lib/ovsdb-data.h > @@ -197,6 +197,9 @@ struct json *ovsdb_datum_to_json(const struct ovsdb_datum *, > const struct ovsdb_type *); > struct json *ovsdb_datum_to_json_deep(const struct ovsdb_datum *, > const struct ovsdb_type *); > +void ovsdb_datum_to_json_ds(const struct ovsdb_datum *, > + const struct ovsdb_type *, > + struct ds *); > > char *ovsdb_datum_from_string(struct ovsdb_datum *, > const struct ovsdb_type *, const char *, > diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c > index c3bfae3d2..8355b8da7 100644 > --- a/ovsdb/monitor.c > +++ b/ovsdb/monitor.c > @@ -25,6 +25,7 @@ > #include "openvswitch/json.h" > #include "json.h" > #include "jsonrpc.h" > +#include "ovsdb-data.h" > #include "ovsdb-error.h" > #include "ovsdb-parser.h" > #include "ovsdb.h" > @@ -197,14 +198,15 @@ enum ovsdb_monitor_row_type { > OVSDB_MONITOR_ROW > }; > > -typedef struct json * > +typedef bool > (*compose_row_update_cb_func) > (const struct ovsdb_monitor_table *mt, > const struct ovsdb_monitor_session_condition * condition, > enum ovsdb_monitor_row_type row_type, > const void *, > bool initial, unsigned long int *changed, > - size_t n_columns); > + size_t n_columns, > + struct ds *); > > static void ovsdb_monitor_destroy(struct ovsdb_monitor *); > static struct ovsdb_monitor_change_set * ovsdb_monitor_add_change_set( > @@ -963,28 +965,27 @@ ovsdb_monitor_row_skip_update(const struct ovsdb_monitor_table *mt, > return false; > } > > -/* Returns JSON for a <row-update> (as described in RFC 7047) for 'row' within > - * 'mt', or NULL if no row update should be sent. > +/* Serializes a <row-update> (as described in RFC 7047) for 'row' within > + * 'mt' into 'ds'. Returns true unless no row update should be sent. > * > - * The caller should specify 'initial' as true if the returned JSON is going to > - * be used as part of the initial reply to a "monitor" request, false if it is > - * going to be used as part of an "update" notification. > + * The caller should specify 'initial' as true if the serialized JSON is > + * going to be used as part of the initial reply to a "monitor" request, > + * false if it is going to be used as part of an "update" notification. > * > * 'changed' must be a scratch buffer for internal use that is at least > * bitmap_n_bytes(n_columns) bytes long. */ > -static struct json * > +static bool > ovsdb_monitor_compose_row_update( > const struct ovsdb_monitor_table *mt, > const struct ovsdb_monitor_session_condition *condition OVS_UNUSED, > enum ovsdb_monitor_row_type row_type OVS_UNUSED, > const void *_row, > bool initial, unsigned long int *changed, > - size_t n_columns OVS_UNUSED) > + size_t n_columns OVS_UNUSED, > + struct ds *ds) > { > const struct ovsdb_monitor_row *row = _row; > enum ovsdb_monitor_selection type; > - struct json *old_json, *new_json; > - struct json *row_json; > size_t i; > > ovs_assert(row_type == OVSDB_MONITOR_ROW); > @@ -992,65 +993,95 @@ ovsdb_monitor_compose_row_update( > if (ovsdb_monitor_row_skip_update(mt, row_type, row->old, > row->new, type, changed, > mt->n_columns)) { > - return NULL; > + return false; > } > > - row_json = json_object_create(); > - old_json = new_json = NULL; > + ds_put_char(ds, '{'); > + > + bool has_old = false; > if (type & (OJMS_DELETE | OJMS_MODIFY)) { > - old_json = json_object_create(); > - json_object_put(row_json, "old", old_json); > - } > - if (type & (OJMS_INITIAL | OJMS_INSERT | OJMS_MODIFY)) { > - new_json = json_object_create(); > - json_object_put(row_json, "new", new_json); > - } > - for (i = 0; i < mt->n_monitored_columns; i++) { > - const struct ovsdb_monitor_column *c = &mt->columns[i]; > + ds_put_cstr(ds, "\"old\":{"); > There is now a lot of duplicate code with only the old/new string being different. Can we create a function to remove the duplication? > - if (!c->monitored || !(type & c->select)) { > - /* We don't care about this type of change for this > - * particular column (but we will care about it for some > - * other column). */ > - continue; > + has_old = true; > + bool empty = true; > + for (i = 0; i < mt->n_monitored_columns; i++) { > + const struct ovsdb_monitor_column *c = &mt->columns[i]; > + > + if (!c->monitored || !(type & c->select)) { > + /* We don't care about this type of change for this > + * particular column (but we will care about it for some > + * other column). */ > + continue; > + } > + > + if (type == OJMS_DELETE || bitmap_is_set(changed, i)) { > + if (!empty) { > + ds_put_char(ds, ','); > + } > + empty = false; > + > + json_string_escape(c->column->name, ds); > + ds_put_char(ds, ':'); > + ovsdb_datum_to_json_ds(&row->old[i], &c->column->type, ds); > + } > } > + ds_put_char(ds, '}'); > + } > > - if ((type == OJMS_MODIFY && bitmap_is_set(changed, i)) > - || type == OJMS_DELETE) { > - json_object_put(old_json, c->column->name, > - ovsdb_datum_to_json(&row->old[i], > - &c->column->type)); > + if (type & (OJMS_INITIAL | OJMS_INSERT | OJMS_MODIFY)) { > + if (has_old) { > + ds_put_char(ds, ','); > } > - if (type & (OJMS_INITIAL | OJMS_INSERT | OJMS_MODIFY)) { > - json_object_put(new_json, c->column->name, > - ovsdb_datum_to_json(&row->new[i], > - &c->column->type)); > + ds_put_cstr(ds, "\"new\":{"); > + > + bool empty = true; > + for (i = 0; i < mt->n_monitored_columns; i++) { > + const struct ovsdb_monitor_column *c = &mt->columns[i]; > + > + if (!c->monitored || !(type & c->select)) { > + /* We don't care about this type of change for this > + * particular column (but we will care about it for some > + * other column). */ > + continue; > + } > + > + if (!empty) { > + ds_put_char(ds, ','); > + } > + empty = false; > + > + json_string_escape(c->column->name, ds); > + ds_put_char(ds, ':'); > + ovsdb_datum_to_json_ds(&row->new[i], &c->column->type, ds); > } > + ds_put_char(ds, '}'); > } > + ds_put_char(ds, '}'); > > - return row_json; > + return true; > } > > -/* Returns JSON for a <row-update2> (as described in ovsdb-server(1) mapage) > - * for 'row' within * 'mt', or NULL if no row update should be sent. > +/* Serializes a <row-update2> (as described in ovsdb-server(1) mapage) > + * for 'row' within * 'mt' into 'ds'. > + * Returns true unless no row update should be sent. > * > - * The caller should specify 'initial' as true if the returned JSON is > + * The caller should specify 'initial' as true if the serialized JSON is > * going to be used as part of the initial reply to a "monitor_cond" request, > * false if it is going to be used as part of an "update2" notification. > * > * 'changed' must be a scratch buffer for internal use that is at least > * bitmap_n_bytes(n_columns) bytes long. */ > -static struct json * > +static bool > ovsdb_monitor_compose_row_update2( > const struct ovsdb_monitor_table *mt, > const struct ovsdb_monitor_session_condition *condition, > enum ovsdb_monitor_row_type row_type, > const void *_row, > bool initial, unsigned long int *changed, > - size_t n_columns) > + size_t n_columns, > + struct ds *ds) > { > enum ovsdb_monitor_selection type; > - struct json *row_update2, *diff_json; > const struct ovsdb_datum *old, *new; > size_t i; > > @@ -1065,15 +1096,18 @@ ovsdb_monitor_compose_row_update2( > row_type, old, new); > if (ovsdb_monitor_row_skip_update(mt, row_type, old, new, type, changed, > n_columns)) { > - return NULL; > + return false; > } > > - row_update2 = json_object_create(); > + ds_put_char(ds, '{'); > if (type == OJMS_DELETE) { > - json_object_put(row_update2, "delete", json_null_create()); > + ds_put_cstr(ds, "\"delete\":null"); > } else { > - diff_json = json_object_create(); > const char *op; > + op = type == OJMS_INITIAL ? "initial" > + : type == OJMS_MODIFY ? "modify" : "insert"; > + ds_put_format(ds, "\"%s\":{", op); > + bool empty = true; > > for (i = 0; i < mt->n_monitored_columns; i++) { > const struct ovsdb_monitor_column *c = &mt->columns[i]; > @@ -1085,33 +1119,43 @@ ovsdb_monitor_compose_row_update2( > continue; > } > > - if (type == OJMS_MODIFY) { > - struct ovsdb_datum diff; > + struct ovsdb_datum diff; > + const struct ovsdb_datum *column_update = NULL; > > + if (type == OJMS_MODIFY) { > if (!bitmap_is_set(changed, i)) { > continue; > } > > - ovsdb_datum_diff(&diff ,&old[index], &new[index], > + ovsdb_datum_diff(&diff, &old[index], &new[index], > &c->column->type); > - json_object_put(diff_json, c->column->name, > - ovsdb_datum_to_json(&diff, &c->column->type)); > - ovsdb_datum_destroy(&diff, &c->column->type); > + column_update = &diff; > } else { > - if (!ovsdb_datum_is_default(&new[index], &c->column->type)) { > - json_object_put(diff_json, c->column->name, > - ovsdb_datum_to_json(&new[index], > - &c->column->type)); > + if (ovsdb_datum_is_default(&new[index], &c->column->type)) { > + continue; > } > + > + column_update = &new[index]; > + } > + > + if (!empty) { > + ds_put_char(ds, ','); > + } > + empty = false; > + json_string_escape(c->column->name, ds); > + ds_put_char(ds, ':'); > + ovsdb_datum_to_json_ds(column_update, &c->column->type, ds); > + > + if (type == OJMS_MODIFY) { > + ovsdb_datum_destroy(&diff, &c->column->type); > } > } > > - op = type == OJMS_INITIAL ? "initial" > - : type == OJMS_MODIFY ? "modify" : "insert"; > - json_object_put(row_update2, op, diff_json); > + ds_put_char(ds, '}'); > } > + ds_put_char(ds, '}'); > > - return row_update2; > + return true; > } > > static size_t > @@ -1166,6 +1210,8 @@ ovsdb_monitor_compose_update( Same comments here, as for ovsdb_monitor_compose_cond_change_update() below (sorry I started with the wrong function). > size_t max_columns = ovsdb_monitor_max_columns(dbmon); > unsigned long int *changed = xmalloc(bitmap_n_bytes(max_columns)); > > + struct ds ds; > + ds_init(&ds); > json = NULL; > struct ovsdb_monitor_change_set_for_table *mcst; > LIST_FOR_EACH (mcst, list_in_change_set, &mcs->change_set_for_tables) { > @@ -1176,16 +1222,24 @@ ovsdb_monitor_compose_update( > HMAP_FOR_EACH_SAFE (row, hmap_node, &mcst->rows) { > cooperative_multitasking_yield(); > > - struct json *row_json; > - row_json = (*row_update)(mt, condition, OVSDB_MONITOR_ROW, row, > - initial, changed, mcst->n_columns); > - if (row_json) { > + bool have_update; > + have_update = (*row_update)(mt, condition, OVSDB_MONITOR_ROW, row, > + initial, changed, mcst->n_columns, > + &ds); > + if (have_update) { > + struct json *row_json; > + /* row_json content is only used for serialization */ > + row_json = > + json_serialized_object_create_from_string(ds_cstr_ro(&ds)); > + ds_clear(&ds); > + > ovsdb_monitor_add_json_row(&json, mt->table->schema->name, > &table_json, row_json, > &row->uuid); > } > } > } > + ds_destroy(&ds); > free(changed); > > return json; > @@ -1201,6 +1255,9 @@ ovsdb_monitor_compose_cond_change_update( > size_t max_columns = ovsdb_monitor_max_columns(dbmon); > unsigned long int *changed = xmalloc(bitmap_n_bytes(max_columns)); > > + struct ds ds; > + ds_init(&ds); Maybe do ‘struct ds ds = DS_EMPTY_INITIALIZER;’ and move it higher at the right place (reverse Christmas tree order) with the other definitions. > + > SHASH_FOR_EACH (node, &dbmon->tables) { > struct ovsdb_condition *old_condition, *new_condition, *diff_condition; > struct ovsdb_monitor_table *mt = node->data; > @@ -1219,15 +1276,21 @@ ovsdb_monitor_compose_cond_change_update( > > /* Iterate over all rows in table */ > HMAP_FOR_EACH (row, hmap_node, &mt->table->rows) { > - struct json *row_json; > - > cooperative_multitasking_yield(); > > - row_json = ovsdb_monitor_compose_row_update2(mt, condition, > - OVSDB_ROW, row, > - false, changed, > - mt->n_columns); > - if (row_json) { > + bool have_update; > + have_update = ovsdb_monitor_compose_row_update2(mt, condition, > + OVSDB_ROW, row, > + false, changed, > + mt->n_columns, > + &ds); > + if (have_update) { > + struct json *row_json; > + /* row_json content is only used for serialization */ > + row_json = > + json_serialized_object_create_from_string(ds_cstr_ro(&ds)); Maybe create a function in the likes of json_serialized_object_create_from_string_nocopy() (see json_string_create_nocopy()), and use ds_steal_cstr() so we do not need to allocate and copy the memory. This should improve your results even more ;) > + ds_clear(&ds); Not needed if you use ds_steal_cstr() > + > ovsdb_monitor_add_json_row(&json, mt->table->schema->name, > &table_json, row_json, > ovsdb_row_get_uuid(row)); > @@ -1235,6 +1298,7 @@ ovsdb_monitor_compose_cond_change_update( > } > ovsdb_monitor_table_condition_updated(mt, condition); > } > + ds_destroy(&ds); > free(changed); > > return json; > -- > 2.45.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h index 555440760..80b9479c7 100644 --- a/include/openvswitch/json.h +++ b/include/openvswitch/json.h @@ -81,6 +81,7 @@ struct json *json_boolean_create(bool); struct json *json_string_create(const char *); struct json *json_string_create_nocopy(char *); struct json *json_serialized_object_create(const struct json *); +struct json *json_serialized_object_create_from_string(const char *); struct json *json_integer_create(long long int); struct json *json_real_create(double); diff --git a/lib/json.c b/lib/json.c index d40e93857..66b1f571f 100644 --- a/lib/json.c +++ b/lib/json.c @@ -199,6 +199,14 @@ json_serialized_object_create(const struct json *src) return json; } +struct json * +json_serialized_object_create_from_string(const char *s) +{ + struct json *json = json_create(JSON_SERIALIZED_OBJECT); + json->string = xstrdup(s); + return json; +} + struct json * json_serialized_object_create_with_yield(const struct json *src) { diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c index defb048d7..f32b7975a 100644 --- a/lib/ovsdb-data.c +++ b/lib/ovsdb-data.c @@ -492,6 +492,45 @@ ovsdb_atom_to_json__(const union ovsdb_atom *atom, enum ovsdb_atomic_type type, } } +/* Serializes 'atom', of the specified 'type', into 'ds' in JSON format. + * + * Refer to RFC 7047 for the format of the JSON that this function produces. */ +static void +ovsdb_atom_to_json_ds(const union ovsdb_atom *atom, + enum ovsdb_atomic_type type, struct ds *ds) +{ + switch (type) { + case OVSDB_TYPE_VOID: + OVS_NOT_REACHED(); + + case OVSDB_TYPE_INTEGER: + ds_put_format(ds, "%lld", (long long) atom->integer); + return; + + case OVSDB_TYPE_REAL: + ds_put_format(ds, "%.*g", DBL_DIG, atom->real); + return; + + case OVSDB_TYPE_BOOLEAN: + ds_put_cstr(ds, atom->boolean ? "true" : "false"); + return; + + case OVSDB_TYPE_STRING: + json_to_ds(atom->s, JSSF_SORT, ds); + return; + + case OVSDB_TYPE_UUID: + ds_put_cstr(ds, "[\"uuid\",\""); + ds_put_format(ds, UUID_FMT, UUID_ARGS(&atom->uuid)); + ds_put_cstr(ds, "\"]"); + return; + + case OVSDB_N_TYPES: + default: + OVS_NOT_REACHED(); + } +} + struct json * ovsdb_atom_to_json(const union ovsdb_atom *atom, enum ovsdb_atomic_type type) { @@ -1445,6 +1484,23 @@ ovsdb_base_to_json(const union ovsdb_atom *atom, } } +static void +ovsdb_base_to_json_ds(const union ovsdb_atom *atom, + const struct ovsdb_base_type *base, + bool use_row_names, + struct ds *ds) +{ + if (!use_row_names + || base->type != OVSDB_TYPE_UUID + || !base->uuid.refTableName) { + ovsdb_atom_to_json_ds(atom, base->type, ds); + } else { + ds_put_cstr(ds, "[\"named-uuid\",\""); + ds_put_format(ds, UUID_ROW_FMT, UUID_ARGS(&atom->uuid)); + ds_put_cstr(ds, "\"]"); + } +} + static struct json * ovsdb_datum_to_json__(const struct ovsdb_datum *datum, const struct ovsdb_type *type, @@ -1482,6 +1538,47 @@ ovsdb_datum_to_json__(const struct ovsdb_datum *datum, } } +static void +ovsdb_datum_to_json_ds__(const struct ovsdb_datum *datum, + const struct ovsdb_type *type, + bool use_row_names, + struct ds *ds) +{ + if (ovsdb_type_is_map(type)) { + size_t i; + + ds_put_cstr(ds, "[\"map\",["); + for (i = 0; i < datum->n; i++) { + if (i != 0) { + ds_put_char(ds, ','); + } + ds_put_char(ds, '['); + ovsdb_base_to_json_ds(&datum->keys[i], &type->key, + use_row_names, ds); + ds_put_char(ds, ','); + ovsdb_base_to_json_ds(&datum->values[i], &type->value, + use_row_names, ds); + ds_put_char(ds, ']'); + } + ds_put_cstr(ds, "]]"); + } else if (datum->n == 1) { + ovsdb_base_to_json_ds(&datum->keys[0], &type->key, + use_row_names, ds); + } else { + size_t i; + + ds_put_cstr(ds, "[\"set\",["); + for (i = 0; i < datum->n; i++) { + if (i != 0) { + ds_put_char(ds, ','); + } + ovsdb_base_to_json_ds(&datum->keys[i], &type->key, + use_row_names, ds); + } + ds_put_cstr(ds, "]]"); + } +} + /* Converts 'datum', of the specified 'type', to JSON format, and returns the * JSON. The caller is responsible for freeing the returned JSON. * @@ -1509,6 +1606,14 @@ ovsdb_datum_to_json_with_row_names(const struct ovsdb_datum *datum, return ovsdb_datum_to_json__(datum, type, true, true); } +void +ovsdb_datum_to_json_ds(const struct ovsdb_datum *datum, + const struct ovsdb_type *type, + struct ds *ds) +{ + ovsdb_datum_to_json_ds__(datum, type, false, ds); +} + static const char * skip_spaces(const char *p) { diff --git a/lib/ovsdb-data.h b/lib/ovsdb-data.h index c0408ee49..5bc47f0a1 100644 --- a/lib/ovsdb-data.h +++ b/lib/ovsdb-data.h @@ -197,6 +197,9 @@ struct json *ovsdb_datum_to_json(const struct ovsdb_datum *, const struct ovsdb_type *); struct json *ovsdb_datum_to_json_deep(const struct ovsdb_datum *, const struct ovsdb_type *); +void ovsdb_datum_to_json_ds(const struct ovsdb_datum *, + const struct ovsdb_type *, + struct ds *); char *ovsdb_datum_from_string(struct ovsdb_datum *, const struct ovsdb_type *, const char *, diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c index c3bfae3d2..8355b8da7 100644 --- a/ovsdb/monitor.c +++ b/ovsdb/monitor.c @@ -25,6 +25,7 @@ #include "openvswitch/json.h" #include "json.h" #include "jsonrpc.h" +#include "ovsdb-data.h" #include "ovsdb-error.h" #include "ovsdb-parser.h" #include "ovsdb.h" @@ -197,14 +198,15 @@ enum ovsdb_monitor_row_type { OVSDB_MONITOR_ROW }; -typedef struct json * +typedef bool (*compose_row_update_cb_func) (const struct ovsdb_monitor_table *mt, const struct ovsdb_monitor_session_condition * condition, enum ovsdb_monitor_row_type row_type, const void *, bool initial, unsigned long int *changed, - size_t n_columns); + size_t n_columns, + struct ds *); static void ovsdb_monitor_destroy(struct ovsdb_monitor *); static struct ovsdb_monitor_change_set * ovsdb_monitor_add_change_set( @@ -963,28 +965,27 @@ ovsdb_monitor_row_skip_update(const struct ovsdb_monitor_table *mt, return false; } -/* Returns JSON for a <row-update> (as described in RFC 7047) for 'row' within - * 'mt', or NULL if no row update should be sent. +/* Serializes a <row-update> (as described in RFC 7047) for 'row' within + * 'mt' into 'ds'. Returns true unless no row update should be sent. * - * The caller should specify 'initial' as true if the returned JSON is going to - * be used as part of the initial reply to a "monitor" request, false if it is - * going to be used as part of an "update" notification. + * The caller should specify 'initial' as true if the serialized JSON is + * going to be used as part of the initial reply to a "monitor" request, + * false if it is going to be used as part of an "update" notification. * * 'changed' must be a scratch buffer for internal use that is at least * bitmap_n_bytes(n_columns) bytes long. */ -static struct json * +static bool ovsdb_monitor_compose_row_update( const struct ovsdb_monitor_table *mt, const struct ovsdb_monitor_session_condition *condition OVS_UNUSED, enum ovsdb_monitor_row_type row_type OVS_UNUSED, const void *_row, bool initial, unsigned long int *changed, - size_t n_columns OVS_UNUSED) + size_t n_columns OVS_UNUSED, + struct ds *ds) { const struct ovsdb_monitor_row *row = _row; enum ovsdb_monitor_selection type; - struct json *old_json, *new_json; - struct json *row_json; size_t i; ovs_assert(row_type == OVSDB_MONITOR_ROW); @@ -992,65 +993,95 @@ ovsdb_monitor_compose_row_update( if (ovsdb_monitor_row_skip_update(mt, row_type, row->old, row->new, type, changed, mt->n_columns)) { - return NULL; + return false; } - row_json = json_object_create(); - old_json = new_json = NULL; + ds_put_char(ds, '{'); + + bool has_old = false; if (type & (OJMS_DELETE | OJMS_MODIFY)) { - old_json = json_object_create(); - json_object_put(row_json, "old", old_json); - } - if (type & (OJMS_INITIAL | OJMS_INSERT | OJMS_MODIFY)) { - new_json = json_object_create(); - json_object_put(row_json, "new", new_json); - } - for (i = 0; i < mt->n_monitored_columns; i++) { - const struct ovsdb_monitor_column *c = &mt->columns[i]; + ds_put_cstr(ds, "\"old\":{"); - if (!c->monitored || !(type & c->select)) { - /* We don't care about this type of change for this - * particular column (but we will care about it for some - * other column). */ - continue; + has_old = true; + bool empty = true; + for (i = 0; i < mt->n_monitored_columns; i++) { + const struct ovsdb_monitor_column *c = &mt->columns[i]; + + if (!c->monitored || !(type & c->select)) { + /* We don't care about this type of change for this + * particular column (but we will care about it for some + * other column). */ + continue; + } + + if (type == OJMS_DELETE || bitmap_is_set(changed, i)) { + if (!empty) { + ds_put_char(ds, ','); + } + empty = false; + + json_string_escape(c->column->name, ds); + ds_put_char(ds, ':'); + ovsdb_datum_to_json_ds(&row->old[i], &c->column->type, ds); + } } + ds_put_char(ds, '}'); + } - if ((type == OJMS_MODIFY && bitmap_is_set(changed, i)) - || type == OJMS_DELETE) { - json_object_put(old_json, c->column->name, - ovsdb_datum_to_json(&row->old[i], - &c->column->type)); + if (type & (OJMS_INITIAL | OJMS_INSERT | OJMS_MODIFY)) { + if (has_old) { + ds_put_char(ds, ','); } - if (type & (OJMS_INITIAL | OJMS_INSERT | OJMS_MODIFY)) { - json_object_put(new_json, c->column->name, - ovsdb_datum_to_json(&row->new[i], - &c->column->type)); + ds_put_cstr(ds, "\"new\":{"); + + bool empty = true; + for (i = 0; i < mt->n_monitored_columns; i++) { + const struct ovsdb_monitor_column *c = &mt->columns[i]; + + if (!c->monitored || !(type & c->select)) { + /* We don't care about this type of change for this + * particular column (but we will care about it for some + * other column). */ + continue; + } + + if (!empty) { + ds_put_char(ds, ','); + } + empty = false; + + json_string_escape(c->column->name, ds); + ds_put_char(ds, ':'); + ovsdb_datum_to_json_ds(&row->new[i], &c->column->type, ds); } + ds_put_char(ds, '}'); } + ds_put_char(ds, '}'); - return row_json; + return true; } -/* Returns JSON for a <row-update2> (as described in ovsdb-server(1) mapage) - * for 'row' within * 'mt', or NULL if no row update should be sent. +/* Serializes a <row-update2> (as described in ovsdb-server(1) mapage) + * for 'row' within * 'mt' into 'ds'. + * Returns true unless no row update should be sent. * - * The caller should specify 'initial' as true if the returned JSON is + * The caller should specify 'initial' as true if the serialized JSON is * going to be used as part of the initial reply to a "monitor_cond" request, * false if it is going to be used as part of an "update2" notification. * * 'changed' must be a scratch buffer for internal use that is at least * bitmap_n_bytes(n_columns) bytes long. */ -static struct json * +static bool ovsdb_monitor_compose_row_update2( const struct ovsdb_monitor_table *mt, const struct ovsdb_monitor_session_condition *condition, enum ovsdb_monitor_row_type row_type, const void *_row, bool initial, unsigned long int *changed, - size_t n_columns) + size_t n_columns, + struct ds *ds) { enum ovsdb_monitor_selection type; - struct json *row_update2, *diff_json; const struct ovsdb_datum *old, *new; size_t i; @@ -1065,15 +1096,18 @@ ovsdb_monitor_compose_row_update2( row_type, old, new); if (ovsdb_monitor_row_skip_update(mt, row_type, old, new, type, changed, n_columns)) { - return NULL; + return false; } - row_update2 = json_object_create(); + ds_put_char(ds, '{'); if (type == OJMS_DELETE) { - json_object_put(row_update2, "delete", json_null_create()); + ds_put_cstr(ds, "\"delete\":null"); } else { - diff_json = json_object_create(); const char *op; + op = type == OJMS_INITIAL ? "initial" + : type == OJMS_MODIFY ? "modify" : "insert"; + ds_put_format(ds, "\"%s\":{", op); + bool empty = true; for (i = 0; i < mt->n_monitored_columns; i++) { const struct ovsdb_monitor_column *c = &mt->columns[i]; @@ -1085,33 +1119,43 @@ ovsdb_monitor_compose_row_update2( continue; } - if (type == OJMS_MODIFY) { - struct ovsdb_datum diff; + struct ovsdb_datum diff; + const struct ovsdb_datum *column_update = NULL; + if (type == OJMS_MODIFY) { if (!bitmap_is_set(changed, i)) { continue; } - ovsdb_datum_diff(&diff ,&old[index], &new[index], + ovsdb_datum_diff(&diff, &old[index], &new[index], &c->column->type); - json_object_put(diff_json, c->column->name, - ovsdb_datum_to_json(&diff, &c->column->type)); - ovsdb_datum_destroy(&diff, &c->column->type); + column_update = &diff; } else { - if (!ovsdb_datum_is_default(&new[index], &c->column->type)) { - json_object_put(diff_json, c->column->name, - ovsdb_datum_to_json(&new[index], - &c->column->type)); + if (ovsdb_datum_is_default(&new[index], &c->column->type)) { + continue; } + + column_update = &new[index]; + } + + if (!empty) { + ds_put_char(ds, ','); + } + empty = false; + json_string_escape(c->column->name, ds); + ds_put_char(ds, ':'); + ovsdb_datum_to_json_ds(column_update, &c->column->type, ds); + + if (type == OJMS_MODIFY) { + ovsdb_datum_destroy(&diff, &c->column->type); } } - op = type == OJMS_INITIAL ? "initial" - : type == OJMS_MODIFY ? "modify" : "insert"; - json_object_put(row_update2, op, diff_json); + ds_put_char(ds, '}'); } + ds_put_char(ds, '}'); - return row_update2; + return true; } static size_t @@ -1166,6 +1210,8 @@ ovsdb_monitor_compose_update( size_t max_columns = ovsdb_monitor_max_columns(dbmon); unsigned long int *changed = xmalloc(bitmap_n_bytes(max_columns)); + struct ds ds; + ds_init(&ds); json = NULL; struct ovsdb_monitor_change_set_for_table *mcst; LIST_FOR_EACH (mcst, list_in_change_set, &mcs->change_set_for_tables) { @@ -1176,16 +1222,24 @@ ovsdb_monitor_compose_update( HMAP_FOR_EACH_SAFE (row, hmap_node, &mcst->rows) { cooperative_multitasking_yield(); - struct json *row_json; - row_json = (*row_update)(mt, condition, OVSDB_MONITOR_ROW, row, - initial, changed, mcst->n_columns); - if (row_json) { + bool have_update; + have_update = (*row_update)(mt, condition, OVSDB_MONITOR_ROW, row, + initial, changed, mcst->n_columns, + &ds); + if (have_update) { + struct json *row_json; + /* row_json content is only used for serialization */ + row_json = + json_serialized_object_create_from_string(ds_cstr_ro(&ds)); + ds_clear(&ds); + ovsdb_monitor_add_json_row(&json, mt->table->schema->name, &table_json, row_json, &row->uuid); } } } + ds_destroy(&ds); free(changed); return json; @@ -1201,6 +1255,9 @@ ovsdb_monitor_compose_cond_change_update( size_t max_columns = ovsdb_monitor_max_columns(dbmon); unsigned long int *changed = xmalloc(bitmap_n_bytes(max_columns)); + struct ds ds; + ds_init(&ds); + SHASH_FOR_EACH (node, &dbmon->tables) { struct ovsdb_condition *old_condition, *new_condition, *diff_condition; struct ovsdb_monitor_table *mt = node->data; @@ -1219,15 +1276,21 @@ ovsdb_monitor_compose_cond_change_update( /* Iterate over all rows in table */ HMAP_FOR_EACH (row, hmap_node, &mt->table->rows) { - struct json *row_json; - cooperative_multitasking_yield(); - row_json = ovsdb_monitor_compose_row_update2(mt, condition, - OVSDB_ROW, row, - false, changed, - mt->n_columns); - if (row_json) { + bool have_update; + have_update = ovsdb_monitor_compose_row_update2(mt, condition, + OVSDB_ROW, row, + false, changed, + mt->n_columns, + &ds); + if (have_update) { + struct json *row_json; + /* row_json content is only used for serialization */ + row_json = + json_serialized_object_create_from_string(ds_cstr_ro(&ds)); + ds_clear(&ds); + ovsdb_monitor_add_json_row(&json, mt->table->schema->name, &table_json, row_json, ovsdb_row_get_uuid(row)); @@ -1235,6 +1298,7 @@ ovsdb_monitor_compose_cond_change_update( } ovsdb_monitor_table_condition_updated(mt, condition); } + ds_destroy(&ds); free(changed); return json;
Currently serialization is performed by first converting the internal data representation into JSON objects, followed by serializing these objects by jsonrpc. This process results in lots of allocations for these intermediate objects. Consequently, this not only increases peak memory consumption, but also demands significantly more CPU work. By forming row-update JSONs directly in `ds`, which is then used to create 'serialized object' JSONs, the overall speed increased by a factor of 2.3. A local benchmark was run on a proprietary Southbound backup. Both versions, before and after applying the patch, were measured. For each version, there were two runs with 10 parallel clients, and two runs with 30 parallel clients. CPU time was recorded after startup (before clients started running) and after all clients received all updates. Clients were essentially running `ovsdb-client monitor-cond unix:pipe OVN_Southbound '[[\"actions\",\"!=\",\"sdfdfsd\"]]' Logical_Flow`. Similar results were obtained with other requests that required a significant amount of data transfer. The backup size is about 600 MB. Results are measured in seconds. Before After Baseline x10: 9.53 108.54 Baseline x10: 9.62 108.67 Baseline x30: 9.69 307.04 Baseline x30: 9.65 303.32 Patch x10: 9.67 52.57 Patch x10: 9.57 53.12 Patch x30: 9.53 136.33 Patch x30: 9.63 135.88 Signed-off-by: Grigorii Nazarov <whitecrowbar@gmail.com> --- v2: updated title v3: fixed bracing v4: changed patch number from 4/4 to 3/3 include/openvswitch/json.h | 1 + lib/json.c | 8 ++ lib/ovsdb-data.c | 105 +++++++++++++++++++ lib/ovsdb-data.h | 3 + ovsdb/monitor.c | 210 ++++++++++++++++++++++++------------- 5 files changed, 254 insertions(+), 73 deletions(-)