| Message ID | AT5PR84MB02102B342FB9F085200FA1AFDC130@AT5PR84MB0210.NAMPRD84.PROD.OUTLOOK.COM |
|---|---|
| State | Superseded |
| Headers | show |
"dev" <dev-bounces@openvswitch.org> wrote on 08/16/2016 04:50:37 PM: > From: "Rodriguez Betancourt, Esteban" <estebarb@hpe.com> > To: "dev@openvswitch.org" <dev@openvswitch.org> > Date: 08/16/2016 04:50 PM > Subject: [ovs-dev] [PATCH] json: Use reference counting in json objects > Sent by: "dev" <dev-bounces@openvswitch.org> > > After profiling OVSDB insert performance it was found > that some significant portion of its time OVSDB is > calling the function json_clone. > > Also, most of the usages of json_clone never modify the json, > just keeps it to prevent it to be freed. > > With that in mind the struct json, json_create, json_clone > and json_destroy were modified to keep a count of how many > references of the json struct are left. Only when that count > reaches zero the json struct is freed. > > The old "json_clone" function was renamed as "json_deep_clone". > > The change provides some performance improvement, depending > on the transactions performed in OVSDB. > > Signed-off-by: Esteban Rodriguez Betancourt <estebarb@hpe.com> > --- Can you add some quantification of the statement "provides some performance improvement"?
For example: 50 process inserting each 1000 rows to a test table (4 columns: string * 2, bool, integer) using commit block Master OVS Test Duration 131 seconds Average Inserts Per second 746.2687 inserts/s Average Insert Duration 134.1382ms Minimal Insert Duration 0.166202ms Maximum Insert Duration 489.8593ms JSON GC Patch Test Duration 86 seconds Average Inserts Per second 1176 inserts/s Average Insert Duration 82.26761ms Minimal Insert Duration 0.165448ms Maximum Insert Duration 751.2111ms 5 process inserting 10000 rows each to the same test table Master OVS Test Duration 8 seconds Average Inserts Per second 7142.857 inserts/s Average Insert Duration 0.656431ms Minimal Insert Duration 0.125197ms Maximum Insert Duration 11.93203ms JSON GC Patch Test Duration 7 seconds Average Inserts Per second 8333.333 inserts/s Average Insert Duration 0.55688ms Minimal Insert Duration 0.143233ms Maximum Insert Duration 26.26319ms Also, accordingly with profiling using callgrind functions like json_clone or json_destroy sometimes can take up to 20% and 36% of the ops executed inOVSDB (those functions and functions called by those ones...). Although, in one odd case it shows that json_clone takes 50% of the operations executed. We can't avoid calling json_destroy sometimes, but I didn't find any place where a cloned JSON object is updated: only cloned, passed the new pointer and release the old one. Regards, Esteban From: Ryan Moats [mailto:rmoats@us.ibm.com] Sent: jueves, 25 de agosto de 2016 9:28 To: Rodriguez Betancourt, Esteban <estebarb@hpe.com> Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] json: Use reference counting in json objects "dev" <dev-bounces@openvswitch.org> wrote on 08/16/2016 04:50:37 PM: > From: "Rodriguez Betancourt, Esteban" <estebarb@hpe.com> > To: "dev@openvswitch.org" <dev@openvswitch.org> > Date: 08/16/2016 04:50 PM > Subject: [ovs-dev] [PATCH] json: Use reference counting in json objects > Sent by: "dev" <dev-bounces@openvswitch.org> > > After profiling OVSDB insert performance it was found > that some significant portion of its time OVSDB is > calling the function json_clone. > > Also, most of the usages of json_clone never modify the json, > just keeps it to prevent it to be freed. > > With that in mind the struct json, json_create, json_clone > and json_destroy were modified to keep a count of how many > references of the json struct are left. Only when that count > reaches zero the json struct is freed. > > The old "json_clone" function was renamed as "json_deep_clone". > > The change provides some performance improvement, depending > on the transactions performed in OVSDB. > > Signed-off-by: Esteban Rodriguez Betancourt <estebarb@hpe.com> > --- Can you add some quantification of the statement "provides some performance improvement"?
Hi, can you resubmit with performance data in the commit message? It's important information. Thanks, Ben. On Thu, Aug 25, 2016 at 06:31:18PM +0000, Rodriguez Betancourt, Esteban wrote: > For example: > 50 process inserting each 1000 rows to a test table (4 columns: string * 2, bool, integer) using commit block > > Master OVS > Test Duration 131 seconds > Average Inserts Per second 746.2687 inserts/s > Average Insert Duration 134.1382ms > Minimal Insert Duration 0.166202ms > Maximum Insert Duration 489.8593ms > > JSON GC Patch > Test Duration 86 seconds > Average Inserts Per second 1176 inserts/s > Average Insert Duration 82.26761ms > Minimal Insert Duration 0.165448ms > Maximum Insert Duration 751.2111ms > > 5 process inserting 10000 rows each to the same test table > > Master OVS > Test Duration 8 seconds > Average Inserts Per second 7142.857 inserts/s > Average Insert Duration 0.656431ms > Minimal Insert Duration 0.125197ms > Maximum Insert Duration 11.93203ms > > JSON GC Patch > Test Duration 7 seconds > Average Inserts Per second 8333.333 inserts/s > Average Insert Duration 0.55688ms > Minimal Insert Duration 0.143233ms > Maximum Insert Duration 26.26319ms > > Also, accordingly with profiling using callgrind functions like json_clone or json_destroy sometimes can take up to 20% and 36% of the ops executed inOVSDB (those functions and functions called by those ones...). Although, in one odd case it shows that json_clone takes 50% of the operations executed. We can't avoid calling json_destroy sometimes, but I didn't find any place where a cloned JSON object is updated: only cloned, passed the new pointer and release the old one. > > Regards, > Esteban > > > From: Ryan Moats [mailto:rmoats@us.ibm.com] > Sent: jueves, 25 de agosto de 2016 9:28 > To: Rodriguez Betancourt, Esteban <estebarb@hpe.com> > Cc: dev@openvswitch.org > Subject: Re: [ovs-dev] [PATCH] json: Use reference counting in json objects > > "dev" <dev-bounces@openvswitch.org> wrote on 08/16/2016 04:50:37 PM: > > > From: "Rodriguez Betancourt, Esteban" <estebarb@hpe.com> > > To: "dev@openvswitch.org" <dev@openvswitch.org> > > Date: 08/16/2016 04:50 PM > > Subject: [ovs-dev] [PATCH] json: Use reference counting in json objects > > Sent by: "dev" <dev-bounces@openvswitch.org> > > > > After profiling OVSDB insert performance it was found > > that some significant portion of its time OVSDB is > > calling the function json_clone. > > > > Also, most of the usages of json_clone never modify the json, > > just keeps it to prevent it to be freed. > > > > With that in mind the struct json, json_create, json_clone > > and json_destroy were modified to keep a count of how many > > references of the json struct are left. Only when that count > > reaches zero the json struct is freed. > > > > The old "json_clone" function was renamed as "json_deep_clone". > > > > The change provides some performance improvement, depending > > on the transactions performed in OVSDB. > > > > Signed-off-by: Esteban Rodriguez Betancourt <estebarb@hpe.com> > > --- > > Can you add some quantification of the statement > "provides some performance improvement"? > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev
diff --git a/lib/json.c b/lib/json.c index 4ac250b..20909ed 100644 --- a/lib/json.c +++ b/lib/json.c @@ -333,7 +333,7 @@ static void json_destroy_array(struct json_array *array); void json_destroy(struct json *json) { - if (json) { + if (json && !--json->count) { switch (json->type) { case JSON_OBJECT: json_destroy_object(json->u.object); @@ -390,9 +390,18 @@ json_destroy_array(struct json_array *array) static struct json *json_clone_object(const struct shash *object); static struct json *json_clone_array(const struct json_array *array); +/* Increments the counter of the json instance */ +struct json * +json_clone(const struct json *json_) +{ + struct json *json = CONST_CAST(struct json *, json_); + json->count++; + return json; +} + /* Returns a deep copy of 'json'. */ struct json * -json_clone(const struct json *json) +json_deep_clone(const struct json *json) { switch (json->type) { case JSON_OBJECT: @@ -547,6 +556,10 @@ json_equal_array(const struct json_array *a, const struct json_array *b) bool json_equal(const struct json *a, const struct json *b) { + if (a == b) { + return true; + } + if (a->type != b->type) { return false; } @@ -1405,6 +1418,7 @@ json_create(enum json_type type) { struct json *json = xmalloc(sizeof *json); json->type = type; + json->count = 1; return json; } diff --git a/lib/json.h b/lib/json.h index 3497035..a76c55d 100644 --- a/lib/json.h +++ b/lib/json.h @@ -61,6 +61,7 @@ struct json_array { /* A JSON value. */ struct json { + size_t count; enum json_type type; union { struct shash *object; /* Contains "struct json *"s. */ @@ -99,6 +100,7 @@ double json_real(const struct json *); int64_t json_integer(const struct json *); struct json *json_clone(const struct json *); +struct json *json_deep_clone(const struct json *); void json_destroy(struct json *); size_t json_hash(const struct json *, size_t basis);
After profiling OVSDB insert performance it was found that some significant portion of its time OVSDB is calling the function json_clone. Also, most of the usages of json_clone never modify the json, just keeps it to prevent it to be freed. With that in mind the struct json, json_create, json_clone and json_destroy were modified to keep a count of how many references of the json struct are left. Only when that count reaches zero the json struct is freed. The old "json_clone" function was renamed as "json_deep_clone". The change provides some performance improvement, depending on the transactions performed in OVSDB. Signed-off-by: Esteban Rodriguez Betancourt <estebarb@hpe.com> --- lib/json.c | 18 ++++++++++++++++-- lib/json.h | 2 ++ 2 files changed, 18 insertions(+), 2 deletions(-)