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 <whitecrow...@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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
