On 4/24/25 1:32 AM, Dmitry Porokh via dev wrote:
> According to profiler on some loads UUID to string is pretty common
> operation and almost always it in result calls xasprintf which calls
> vsnprintf twice 1. to calculate length of resulting string 2. to print
> result to string. This patch introduces specialized function for UUID
> printing and both reduces code duplication and improves performance
> exploiting fixed size of output result. For example, on my laptop
> 10'000'000 calls of uuid_to_string function take 1296 ms when
> 10'000'000 calls of xasprintf with UUID_FMT take 2498 ms.
>
> Signed-off-by: Dmitry Porokh <[email protected]>
> ---
> v2 fixes:
> * Commit summary message.
> * Micro-benchmark numbers of performance improvement claims.
> * Two coding style fixes (spaces).
>
> include/openvswitch/dynamic-string.h | 3 +++
> include/openvswitch/json.h | 3 +++
> lib/db-ctl-base.c | 6 +++---
> lib/dynamic-string.c | 8 ++++++++
> lib/json.c | 12 ++++++++++++
> lib/ovsdb-cs.c | 3 +--
> lib/ovsdb-data.c | 5 ++---
> lib/ovsdb-idl.c | 6 ++----
> lib/uuid.h | 14 ++++++++++++++
> ovsdb/jsonrpc-server.c | 21 +++++++--------------
> ovsdb/ovsdb-client.c | 3 +--
> ovsdb/raft-private.c | 17 +++++++----------
> ovsdb/raft-rpc.c | 23 ++++++++++-------------
> ovsdb/raft.c | 6 +++---
> 14 files changed, 76 insertions(+), 54 deletions(-)
>
I n addition to what Eelco already mentioned, see a few more small
comments below.
Best regards, Ilya Maximets.
> diff --git a/include/openvswitch/dynamic-string.h
> b/include/openvswitch/dynamic-string.h
> index 1c262b049..a1d341d48 100644
> --- a/include/openvswitch/dynamic-string.h
> +++ b/include/openvswitch/dynamic-string.h
> @@ -28,6 +28,8 @@
> extern "C" {
> #endif
>
> +struct uuid;
> +
> /* A "dynamic string", that is, a buffer that can be used to construct a
> * string across a series of operations that extend or modify it.
> *
> @@ -58,6 +60,7 @@ void ds_put_format(struct ds *, const char *, ...)
> OVS_PRINTF_FORMAT(2, 3);
> void ds_put_format_valist(struct ds *, const char *, va_list)
> OVS_PRINTF_FORMAT(2, 0);
> void ds_put_printable(struct ds *, const char *, size_t);
> +void ds_put_uuid(struct ds *, const struct uuid *);
> void ds_put_hex(struct ds *ds, const void *buf, size_t size);
> void ds_put_hex_dump(struct ds *ds, const void *buf_, size_t size,
> uintptr_t ofs, bool ascii);
> diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h
> index 555440760..43db272ce 100644
> --- a/include/openvswitch/json.h
> +++ b/include/openvswitch/json.h
> @@ -80,6 +80,7 @@ struct json *json_null_create(void);
> struct json *json_boolean_create(bool);
> struct json *json_string_create(const char *);
> struct json *json_string_create_nocopy(char *);
> +struct json *json_string_create_uuid(const struct uuid *);
> struct json *json_serialized_object_create(const struct json *);
> struct json *json_integer_create(long long int);
> struct json *json_real_create(double);
> @@ -101,6 +102,8 @@ void json_object_put_string(struct json *,
> void json_object_put_format(struct json *,
> const char *name, const char *format, ...)
> OVS_PRINTF_FORMAT(3, 4);
> +void json_object_put_uuid(struct json *json, const char *name,
> + const struct uuid *value);
Remove the 'json' and 'value'. Variables with descriptive types do
not need names in prototypes.
>
> const char *json_string(const struct json *);
> const char *json_serialized_object(const struct json *);
> diff --git a/lib/db-ctl-base.c b/lib/db-ctl-base.c
> index de046a4ed..1f157e46c 100644
> --- a/lib/db-ctl-base.c
> +++ b/lib/db-ctl-base.c
> @@ -1785,7 +1785,7 @@ cmd_create(struct ctl_context *ctx)
> return;
> }
> }
> - ds_put_format(&ctx->output, UUID_FMT, UUID_ARGS(&row->uuid));
> + ds_put_uuid(&ctx->output, &row->uuid);
> }
>
> /* This function may be used as the 'postprocess' function for commands that
> @@ -1809,7 +1809,7 @@ post_create(struct ctl_context *ctx)
> real = ovsdb_idl_txn_get_insert_uuid(ctx->txn, &dummy);
> if (real) {
> ds_clear(&ctx->output);
> - ds_put_format(&ctx->output, UUID_FMT, UUID_ARGS(real));
> + ds_put_uuid(&ctx->output, real);
> }
> ds_put_char(&ctx->output, '\n');
> }
> @@ -2153,7 +2153,7 @@ cmd_show_row(struct ctl_context *ctx, const struct
> ovsdb_idl_row *row,
> datum = ovsdb_idl_read(row, show->name_column);
> ovsdb_datum_to_string(datum, &show->name_column->type, &ctx->output);
> } else {
> - ds_put_format(&ctx->output, UUID_FMT, UUID_ARGS(&row->uuid));
> + ds_put_uuid(&ctx->output, &row->uuid);
> }
> ds_put_char(&ctx->output, '\n');
>
> diff --git a/lib/dynamic-string.c b/lib/dynamic-string.c
> index 8e9555a63..258bc0020 100644
> --- a/lib/dynamic-string.c
> +++ b/lib/dynamic-string.c
> @@ -21,6 +21,7 @@
> #include <string.h>
> #include <time.h>
> #include "timeval.h"
> +#include "uuid.h"
> #include "util.h"
>
> /* Initializes 'ds' as an empty string buffer. */
> @@ -188,6 +189,13 @@ ds_put_printable(struct ds *ds, const char *s, size_t n)
> }
> }
>
> +void
> +ds_put_uuid(struct ds *ds, const struct uuid *uuid) {
'{' should be on a new line.
> + char uuid_str[UUID_LEN + 1];
> + sprintf(uuid_str, UUID_FMT, UUID_ARGS(uuid));
Use snprintf instead. It's not strictly necessary here, but it's a better
practice in general.
> + ds_put_buffer(ds, uuid_str, UUID_LEN);
But also, we can avoid an extra copy here by using ds_put_uninit and
then snprintf directly to the ds, instead of first formatting on stack
and then copying.
> +}
> +
> /* Writes the current time with optional millisecond resolution to 'string'
> * based on 'template'.
> * The current time is either localtime or UTC based on 'utc'. */
> diff --git a/lib/json.c b/lib/json.c
> index 001f6e6ab..fa2e25c3a 100644
> --- a/lib/json.c
> +++ b/lib/json.c
> @@ -189,6 +189,11 @@ json_string_create(const char *s)
> return json_string_create_nocopy(xstrdup(s));
> }
>
> +struct json *json_string_create_uuid(const struct uuid *uuid)
> +{
> + return json_string_create_nocopy(uuid_to_string(uuid));
> +}
> +
> struct json *
> json_serialized_object_create(const struct json *src)
> {
> @@ -342,6 +347,13 @@ json_object_put_format(struct json *json,
> va_end(args);
> }
>
> +void json_object_put_uuid(struct json *json,
> + const char *name, const struct uuid *uuid)
> +{
> + json_object_put(json, name,
> + json_string_create_uuid(uuid));
Can be a single line.
> +}
> +
> const char *
> json_string(const struct json *json)
> {
> diff --git a/lib/ovsdb-cs.c b/lib/ovsdb-cs.c
> index b5eda88ad..206722fbd 100644
> --- a/lib/ovsdb-cs.c
> +++ b/lib/ovsdb-cs.c
> @@ -1546,8 +1546,7 @@ ovsdb_cs_send_monitor_request(struct ovsdb_cs *cs,
> struct ovsdb_cs_db *db,
> json_clone(db->monitor_id),
> mrs);
> if (version == 3) {
> - struct json *json_last_id = json_string_create_nocopy(
> - xasprintf(UUID_FMT, UUID_ARGS(&db->last_id)));
> + struct json *json_last_id = json_string_create_uuid(&db->last_id);
> json_array_add(params, json_last_id);
May reduce this to
json_array_add(params, json_string_create_uuid(&db->last_id));
No real need for a variable.
> }
> ovsdb_cs_send_request(cs, jsonrpc_create_request(method, params, NULL));
> diff --git a/lib/ovsdb-data.c b/lib/ovsdb-data.c
> index aadac6bb2..e4149401f 100644
> --- a/lib/ovsdb-data.c
> +++ b/lib/ovsdb-data.c
> @@ -483,8 +483,7 @@ ovsdb_atom_to_json__(const union ovsdb_atom *atom, enum
> ovsdb_atomic_type type,
> : json_deep_clone(atom->s);
>
> case OVSDB_TYPE_UUID:
> - return wrap_json("uuid", json_string_create_nocopy(
> - xasprintf(UUID_FMT, UUID_ARGS(&atom->uuid))));
> + return wrap_json("uuid", json_string_create_uuid(&atom->uuid));
>
> case OVSDB_N_TYPES:
> default:
> @@ -753,7 +752,7 @@ ovsdb_atom_to_string(const union ovsdb_atom *atom, enum
> ovsdb_atomic_type type,
> break;
>
> case OVSDB_TYPE_UUID:
> - ds_put_format(out, UUID_FMT, UUID_ARGS(&atom->uuid));
> + ds_put_uuid(out, &atom->uuid);
> break;
>
> case OVSDB_N_TYPES:
> diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
> index 4c2a3e3aa..226a4b475 100644
> --- a/lib/ovsdb-idl.c
> +++ b/lib/ovsdb-idl.c
> @@ -2838,8 +2838,7 @@ where_uuid_equals(const struct uuid *uuid)
> json_string_create("=="),
> json_array_create_2(
> json_string_create("uuid"),
> - json_string_create_nocopy(
> - xasprintf(UUID_FMT, UUID_ARGS(uuid))))));
> + json_string_create_uuid(uuid))));
> }
>
> static const struct ovsdb_idl_row *
> @@ -3308,8 +3307,7 @@ ovsdb_idl_txn_commit(struct ovsdb_idl_txn *txn)
> struct json *value;
> if (row->persist_uuid) {
> uuid_json = "uuid";
> - value = json_string_create_nocopy(
> - xasprintf(UUID_FMT, UUID_ARGS(&row->uuid)));
> + value = json_string_create_uuid(&row->uuid);
> } else {
> uuid_json = "uuid-name";
> value = json_string_create_nocopy(
> diff --git a/lib/uuid.h b/lib/uuid.h
> index fa49354f6..75156d701 100644
> --- a/lib/uuid.h
> +++ b/lib/uuid.h
> @@ -16,6 +16,7 @@
> #ifndef UUID_H
> #define UUID_H 1
>
> +#include "util.h"
> #include "openvswitch/uuid.h"
>
> #ifdef __cplusplus
> @@ -73,6 +74,19 @@ uuid_prefix(const struct uuid *uuid, int digits)
> return (uuid->parts[0] >> (32 - 4 * digits));
> }
>
> +/* Returns a string representation of UUID.
> + *
> + * String is allocated on heap. Ownership of string
> + * goes to the caller.
> + */
> +static inline char *
> +uuid_to_string(const struct uuid *uuid)
> +{
> + char *data = xmalloc(UUID_LEN + 1);
> + sprintf(data, UUID_FMT, UUID_ARGS(uuid));
snprintf.
> + return data;
> +}
> +
> void uuid_init(void);
> void uuid_generate(struct uuid *);
> struct uuid uuid_random(void);
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 26a53898f..ba5a4211b 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -1157,8 +1157,7 @@ ovsdb_jsonrpc_session_got_request(struct
> ovsdb_jsonrpc_session *s,
> const struct uuid *uuid = &s->up.server->uuid;
> struct json *result;
>
> - result = json_string_create_nocopy(xasprintf(UUID_FMT,
> - UUID_ARGS(uuid)));
> + result = json_string_create_uuid(uuid);
> reply = jsonrpc_create_reply(result, request->id);
> } else if (!strcmp(request->method, "lock")) {
> reply = ovsdb_jsonrpc_session_lock(s, request, OVSDB_LOCK_WAIT);
> @@ -1609,10 +1608,8 @@ ovsdb_jsonrpc_monitor_create(struct
> ovsdb_jsonrpc_session *s, struct ovsdb *db,
> json = json ? json : json_object_create();
>
> if (m->version == OVSDB_MONITOR_V3) {
> - struct json *json_last_id = json_string_create_nocopy(
> - xasprintf(UUID_FMT,
> - UUID_ARGS(ovsdb_monitor_get_last_txnid(
> - m->dbmon))));
> + struct json *json_last_id = json_string_create_uuid(
> + ovsdb_monitor_get_last_txnid(m->dbmon));
>
> struct json *json_found = json_boolean_create(!initial);
> json = json_array_create_3(json_found, json_last_id, json);
> @@ -1752,10 +1749,8 @@ ovsdb_jsonrpc_monitor_cond_change(struct
> ovsdb_jsonrpc_session *s,
> struct jsonrpc_msg *msg;
> struct json *p;
> if (m->version == OVSDB_MONITOR_V3) {
> - struct json *json_last_id = json_string_create_nocopy(
> - xasprintf(UUID_FMT,
> - UUID_ARGS(ovsdb_monitor_get_last_txnid(
> - m->dbmon))));
> + struct json *json_last_id = json_string_create_uuid(
> + ovsdb_monitor_get_last_txnid(m->dbmon));
>
> p = json_array_create_3(json_clone(m->monitor_id), json_last_id,
> update_json);
> @@ -1912,10 +1907,8 @@ ovsdb_jsonrpc_monitor_flush_all(struct
> ovsdb_jsonrpc_session *s)
> struct jsonrpc_msg *msg;
> struct json *params;
> if (m->version == OVSDB_MONITOR_V3) {
> - struct json *json_last_id = json_string_create_nocopy(
> - xasprintf(UUID_FMT,
> - UUID_ARGS(ovsdb_monitor_get_last_txnid(
> - m->dbmon))));
> + struct json *json_last_id = json_string_create_uuid(
> + ovsdb_monitor_get_last_txnid(m->dbmon));
> params = json_array_create_3(json_clone(m->monitor_id),
> json_last_id, json);
> } else {
> diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
> index 3fa1d9afc..5258eccaa 100644
> --- a/ovsdb/ovsdb-client.c
> +++ b/ovsdb/ovsdb-client.c
> @@ -1469,8 +1469,7 @@ do_monitor__(struct jsonrpc *rpc, const char *database,
> break;
> case OVSDB_MONITOR_V3:
> method = "monitor_cond_since";
> - struct json *json_last_id = json_string_create_nocopy(
> - xasprintf(UUID_FMT, UUID_ARGS(last_id)));
> + struct json *json_last_id = json_string_create_uuid(last_id);
> json_array_add(monitor, json_last_id);
> break;
> case OVSDB_MONITOR_VERSION_MAX:
> diff --git a/ovsdb/raft-private.c b/ovsdb/raft-private.c
> index e685c8103..16ae6d572 100644
> --- a/ovsdb/raft-private.c
> +++ b/ovsdb/raft-private.c
> @@ -312,7 +312,7 @@ raft_entry_to_json(const struct raft_entry *e)
> if (raft_entry_has_data(e)) {
> json_object_put(json, "data",
> json_clone(raft_entry_get_serialized_data(e)));
> - json_object_put_format(json, "eid", UUID_FMT, UUID_ARGS(&e->eid));
> + json_object_put_uuid(json, "eid", &e->eid);
> }
> if (e->servers) {
> json_object_put(json, "servers", json_clone(e->servers));
> @@ -509,10 +509,9 @@ raft_header_to_json(const struct raft_header *h)
> {
> struct json *json = json_object_create();
>
> - json_object_put_format(json, "server_id", UUID_FMT, UUID_ARGS(&h->sid));
> + json_object_put_uuid(json, "server_id", &h->sid);
> if (!uuid_is_zero(&h->cid)) {
> - json_object_put_format(json, "cluster_id",
> - UUID_FMT, UUID_ARGS(&h->cid));
> + json_object_put_uuid(json, "cluster_id", &h->cid);
> }
> json_object_put_string(json, "local_address", h->local_address);
> json_object_put_string(json, "name", h->name);
> @@ -532,8 +531,7 @@ raft_header_to_json(const struct raft_header *h)
> json_object_put(json, "prev_data",
> json_clone(raft_entry_get_serialized_data(&h->snap)));
> }
> - json_object_put_format(json, "prev_eid",
> - UUID_FMT, UUID_ARGS(&h->snap.eid));
> + json_object_put_uuid(json, "prev_eid", &h->snap.eid);
> if (h->snap.election_timer) {
> raft_put_uint64(json, "prev_election_timer",
> h->snap.election_timer);
> @@ -689,8 +687,7 @@ raft_record_to_json(const struct raft_record *r)
> raft_put_uint64(json, "election_timer", r->entry.election_timer);
> }
> if (!uuid_is_zero(&r->entry.eid)) {
> - json_object_put_format(json, "eid",
> - UUID_FMT, UUID_ARGS(&r->entry.eid));
> + json_object_put_uuid(json, "eid", &r->entry.eid);
> }
> break;
>
> @@ -700,7 +697,7 @@ raft_record_to_json(const struct raft_record *r)
>
> case RAFT_REC_VOTE:
> raft_put_uint64(json, "term", r->term);
> - json_object_put_format(json, "vote", UUID_FMT, UUID_ARGS(&r->sid));
> + json_object_put_uuid(json, "vote", &r->sid);
> break;
>
> case RAFT_REC_NOTE:
> @@ -713,7 +710,7 @@ raft_record_to_json(const struct raft_record *r)
>
> case RAFT_REC_LEADER:
> raft_put_uint64(json, "term", r->term);
> - json_object_put_format(json, "leader", UUID_FMT, UUID_ARGS(&r->sid));
> + json_object_put_uuid(json, "leader", &r->sid);
> break;
>
> default:
> diff --git a/ovsdb/raft-rpc.c b/ovsdb/raft-rpc.c
> index 27c3aad99..846727c0a 100644
> --- a/ovsdb/raft-rpc.c
> +++ b/ovsdb/raft-rpc.c
> @@ -333,7 +333,7 @@ raft_vote_reply_to_jsonrpc(const struct raft_vote_reply
> *rpy,
> struct json *args)
> {
> raft_put_uint64(args, "term", rpy->term);
> - json_object_put_format(args, "vote", UUID_FMT, UUID_ARGS(&rpy->vote));
> + json_object_put_uuid(args, "vote", &rpy->vote);
> if (rpy->is_prevote) {
> json_object_put(args, "is_prevote", json_boolean_create(true));
> }
> @@ -476,8 +476,7 @@ raft_remove_server_reply_to_jsonrpc(const struct
> raft_remove_server_reply *rpy,
> struct json *args)
> {
> if (!uuid_is_zero(&rpy->target_sid)) {
> - json_object_put_format(args, "target_server",
> - UUID_FMT, UUID_ARGS(&rpy->target_sid));
> + json_object_put_uuid(args, "target_server", &rpy->target_sid);
> }
> json_object_put(args, "success", json_boolean_create(rpy->success));
> }
> @@ -524,8 +523,7 @@ raft_install_snapshot_request_to_jsonrpc(
> raft_put_uint64(args, "last_index", rq->last_index);
> raft_put_uint64(args, "last_term", rq->last_term);
> json_object_put(args, "last_servers", json_clone(rq->last_servers));
> - json_object_put_format(args, "last_eid",
> - UUID_FMT, UUID_ARGS(&rq->last_eid));
> + json_object_put_uuid(args, "last_eid", &rq->last_eid);
> raft_put_uint64(args, "election_timer", rq->election_timer);
>
> json_object_put(args, "data", json_clone(rq->data));
> @@ -636,7 +634,7 @@ static void
> raft_remove_server_request_to_jsonrpc(
> const struct raft_remove_server_request *rq, struct json *args)
> {
> - json_object_put_format(args, "server_id", UUID_FMT, UUID_ARGS(&rq->sid));
> + json_object_put_uuid(args, "server_id", &rq->sid);
> }
>
> static void
> @@ -708,8 +706,8 @@ raft_execute_command_request_to_jsonrpc(
> const struct raft_execute_command_request *rq, struct json *args)
> {
> json_object_put(args, "data", json_clone(rq->data));
> - json_object_put_format(args, "prereq", UUID_FMT, UUID_ARGS(&rq->prereq));
> - json_object_put_format(args, "result", UUID_FMT, UUID_ARGS(&rq->result));
> + json_object_put_uuid(args, "prereq", &rq->prereq);
> + json_object_put_uuid(args, "result", &rq->result);
> }
>
> static void
> @@ -751,7 +749,7 @@ static void
> raft_execute_command_reply_to_jsonrpc(
> const struct raft_execute_command_reply *rpy, struct json *args)
> {
> - json_object_put_format(args, "result", UUID_FMT,
> UUID_ARGS(&rpy->result));
> + json_object_put_uuid(args, "result", &rpy->result);
> json_object_put_string(args, "status",
> raft_command_status_to_string(rpy->status));
> if (rpy->commit_index) {
> @@ -833,13 +831,12 @@ raft_rpc_to_jsonrpc(const struct uuid *cid,
> {
> struct json *args = json_object_create();
> if (!uuid_is_zero(cid)) {
> - json_object_put_format(args, "cluster", UUID_FMT, UUID_ARGS(cid));
> + json_object_put_uuid(args, "cluster", cid);
> }
> if (!uuid_is_zero(&rpc->common.sid)) {
> - json_object_put_format(args, "to", UUID_FMT,
> - UUID_ARGS(&rpc->common.sid));
> + json_object_put_uuid(args, "to", &rpc->common.sid);
> }
> - json_object_put_format(args, "from", UUID_FMT, UUID_ARGS(sid));
> + json_object_put_uuid(args, "from", sid);
> if (rpc->common.comment) {
> json_object_put_string(args, "comment", rpc->common.comment);
> }
> diff --git a/ovsdb/raft.c b/ovsdb/raft.c
> index 7d78f710e..78ae39e84 100644
> --- a/ovsdb/raft.c
> +++ b/ovsdb/raft.c
> @@ -527,7 +527,7 @@ raft_create_cluster(const char *file_name, const char
> *name,
> };
> raft_entry_set_parsed_data(&h.snap, data);
> shash_add_nocopy(json_object(h.snap.servers),
> - xasprintf(UUID_FMT, UUID_ARGS(&h.sid)),
> + uuid_to_string(&h.sid),
> json_string_create(local_address));
> error = ovsdb_log_write_and_free(log, raft_header_to_json(&h));
> raft_header_uninit(&h);
> @@ -4846,7 +4846,7 @@ raft_unixctl_cid(struct unixctl_conn *conn,
> } else if (uuid_is_zero(&raft->cid)) {
> unixctl_command_reply_error(conn, "cluster id not yet known");
> } else {
> - char *uuid = xasprintf(UUID_FMT, UUID_ARGS(&raft->cid));
> + char *uuid = uuid_to_string(&raft->cid);
> unixctl_command_reply(conn, uuid);
> free(uuid);
> }
> @@ -4861,7 +4861,7 @@ raft_unixctl_sid(struct unixctl_conn *conn,
> if (!raft) {
> unixctl_command_reply_error(conn, "unknown cluster");
> } else {
> - char *uuid = xasprintf(UUID_FMT, UUID_ARGS(&raft->sid));
> + char *uuid = uuid_to_string(&raft->sid);
> unixctl_command_reply(conn, uuid);
> free(uuid);
> }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
