On 16 Sep 2024, at 18:33, Grigorii Nazarov wrote:
> `Hi Eelco,
>
> See my comments below.
>
> I'm new to this process, so I guess I'm to address all the issues together
> when we agree on currently mentioned?
You are right Grigorii. After we wrap up the discussion on all patches in the
series you can send out a new revision, which will go through another round of
reviews.
If something is not clear, just let me know.
See my comments below…
> On 9/6/24 5:14 PM, Eelco Chaudron wrote:
>>
>>
>> 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?
>>
>
> Honestly, it took me some time too, so probably I should reformat it somehow.
> Meanwhile, 'before' and 'after' meaning is before and after payload, so the
> time we improve is (After - Before).
>
> So, we have something close to
> Baseline x10 = 99
> Baseline x30 = 294
> Patch x10 = 43
> Patch x30 = 137
>
> Which is about 2.1-2.3 times greater for Baseline vs Patch.
> I had more thorough testing locally, including wall clock, but I think it's
> lost by now.
Now I get it :) And yes you might want to reformat it.
>>> 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()?
>>
>
> This is ovsdb_atom_to_json__ with inlined json creation. Differences from
> ovsdb_atom_to_string are small and are mostly about how UUID is serialized.
Maybe do something like the below to avoid duplication, and use it in both
functions.
static void
ovsdb_atom_to_json_ds__(const union ovsdb_atom *atom,
enum ovsdb_atomic_type type, struct ds *ds,
bool forced_string_quotes, bool bracket_uuid)
{
…
}
>>> +{
>>> + 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);
>>
>
> Agreed.
>
>>> + 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.
>
> This is but ovsdb_base_to_json with json building made in place. I'm not sure
> why is it done this way too. As to reversing logic, the correct way might be
> if (use_row_names
> && base->type == OVSDB_TYPE_UUID
> && base->uuid.refTableName) {
> // write named-uuid
> } else {
> // atom to json
> }
>
You are right, I might not had enough coffee when reviewing this the first time
:(
Looking at this (and the below) again, I see it’s only ever called with
use_row_names = false. We should probably optimize this all out.
So ovsdb_datum_to_json_ds__() would become ovsdb_datum_to_json_ds() with the
bool use_row_names parameter removed.
>>> + 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?
>>
>
> Agreed. I'll address it in the next revision.
>
>>> - 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).
>>
>
> No problem, see corresponding comments below.
>
>>> 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.
>
> Agreed.
>
>>
>>> +
>>> 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 ;)
>>
>
> This way we will lose more. If we steal buffer from ds, on the next iteration
> it will have to be grown again. Which is more expensive than plain copying.
> Moreover, in case of copy we will allocate strictly the needed space. In case
> of stealing we will have excessive tails in every stolen string. This affects
> peak memory consumption.
>
> Possible optimization could be to steal in the end, on the last iteration.
> But at this moment we won't probably have an update, or there will be too
> much of copyings anyway. It's still possible to defer copying till the next
> iteration, so that we can do stealing after all iterations, but I don't
> believe the result is worth the effort.
>
> Meanwhile, one thing could be done. We can accept in
> json_serialized_object_create_from_string both source buffer and length, and
> thus avoid calling strlen, which will improve time. Or we can accept ds
> itself. I think I will do it, though I don't think this is the slowest place
> anymore.
Maybe I need more coffee, but this should not matter in this case as you will
start with a new ds() each round. Let me add some code here, maybe it becomes
more clear what I meant (not tested):
diff --git a/lib/json.c b/lib/json.c
index 66b1f571f..099d6f3fd 100644
--- a/lib/json.c
+++ b/lib/json.c
@@ -206,6 +206,13 @@ json_serialized_object_create_from_string(const char *s)
json->string = xstrdup(s);
return json;
}
+struct json *
+json_serialized_object_create_from_string_nocopy(const char *s)
+{
+ struct json *json = json_create(JSON_SERIALIZED_OBJECT);
+ json->string = s;
+ return json;
+}
struct json *
json_serialized_object_create_with_yield(const struct json *src)
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index 8355b8da7..3e6b3ddd1 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -1230,8 +1230,8 @@ ovsdb_monitor_compose_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);
+ json_serialized_object_create_from_string_nocopy(
+ ds_steal_cstr(ds));
ovsdb_monitor_add_json_row(&json, mt->table->schema->name,
&table_json, row_json,
Now each round you will start with a new (clean) ds, as before, but you do not
have the xstrdup(s). Am I missing something?
>>> + ds_clear(&ds);
>>
>> Not needed if you use ds_steal_cstr()
>
> See above.
>
>>
>>> +
>>> 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
>>
>
> --
> Best,
> Grigorii
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
