On 1/12/24 17:39, Frode Nordahl wrote:
> Hello, Ilya,
>
> This is a partial response as I've come across something that might
> change the approach for this specific patch, and I thought it would be
> pertinent to disclose that discovery as soon as possible.
>
> I'll respond to the rest as I verify the finding and continue working
> through your comments on this patch.
OK.
>
> On Fri, Jan 12, 2024 at 1:26 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>> On 1/10/24 20:29, Frode Nordahl wrote:
>>> Creating and destroying JSON objects may be time consuming.
>>>
>>> Add yielding counterparts of json_serialized_object_create() and
>>> json_destroy__() functions that make use of the cooperative
>>> multitasking module to yield during processing, allowing time
>>> sensitive tasks in other parts of the program to be completed
>>> during processing.
>>>
>>> Signed-off-by: Frode Nordahl <frode.nord...@canonical.com>
>>> ---
>>> include/openvswitch/json.h | 16 ++++++++--
>>> lib/json.c | 63 ++++++++++++++++++++++++++------------
>>> 2 files changed, 56 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h
>>> index 35b403c29..39528f140 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_with_yield(const struct json *);
>>> struct json *json_integer_create(long long int);
>>> struct json *json_real_create(double);
>>>
>>> @@ -137,7 +138,8 @@ struct json *json_from_stream(FILE *stream);
>>>
>>> enum {
>>> JSSF_PRETTY = 1 << 0, /* Multiple lines with indentation, if
>>> true. */
>>> - JSSF_SORT = 1 << 1 /* Object members in sorted order, if
>>> true. */
>>> + JSSF_SORT = 1 << 1, /* Object members in sorted order, if
>>> true. */
>>> + JSSF_YIELD = 1 << 2 /* Yield during processing */
>>
>> This is a public header, but we're referencing a functionality
>> of the internal library that users of a public header can't access.
>
> The flags are publicly available through the use of the
> json_to_string() functions flags argument, which is why I thought it
> would be natural to put this here.
>
> OTOH, I guess it would not be natural for anyone to use it other than
> through the `_with_yield()` front-end, so I see what you mean.
>
>
>> It might be better to create an internal header in lib/ and
>> define this as an internal flag with a higher value (bit 7?).
>> A macro or a single-value enum should be fine.
>>
>> Also, period at the end of a comment.
>>
>>> };
>>> char *json_to_string(const struct json *, int flags);
>>> void json_to_ds(const struct json *, int flags, struct ds *);
>>> @@ -158,14 +160,22 @@ json_clone(const struct json *json_)
>>> return json;
>>> }
>>>
>>> -void json_destroy__(struct json *json);
>>> +void json_destroy__(struct json *json, bool yield);
>>
>> Same technically applies here, but this is an internal__ function,
>> so may be fine to keep it like that, as users are not supposed to
>> call it.
>>
>>>
>>> /* Frees 'json' and everything it points to, recursively. */
>>> static inline void
>>> json_destroy(struct json *json)
>>> {
>>> if (json && !--json->count) {
>>> - json_destroy__(json);
>>> + json_destroy__(json, false);
>>> + }
>>> +}
>>> +
>>> +static inline void
>>> +json_destroy_with_yield(struct json *json)
>>> +{
>>> + if (json && !--json->count) {
>>> + json_destroy__(json, true);
>>> }
>>> }
>>
>> This last function can be moved to the internal header.
>> Along with the prototype of the json_serialized_object_create_with_yield.
>
> If we put these in a private header, ovsdb/monitor.c would have to
> include a "json-private.h", and that does not feel right to me?
Yeah, here we have a different type of 'private' header, that's
why I used the word 'internal' and not 'private'. json-private.h
indeed sounds like something that is only for json-*.c to include,
but I had in mind a header that in just in lib/ and not include/,
i.e. available for internal users to include, but not exported to
users of a shared library.
I suppose we can just create lib/json.h and put this stuff there.
We do have lib/flow.h and include/openvswitch/flow.h, for example.
In this case, ovsdb/monitor.c can include internal json.h.
>
> I'll try to find some way to stash this away though if you want to
> keep it away from the public interface.
>
>> The name is getting very long...
>>
>>>
>>> diff --git a/lib/json.c b/lib/json.c
>>> index aded8bb01..78ebabb18 100644
>>> --- a/lib/json.c
>>> +++ b/lib/json.c
>>> @@ -24,6 +24,7 @@
>>> #include <limits.h>
>>> #include <string.h>
>>>
>>> +#include "cooperative-multitasking.h"
>>> #include "openvswitch/dynamic-string.h"
>>> #include "hash.h"
>>> #include "openvswitch/shash.h"
>>> @@ -181,14 +182,26 @@ json_string_create(const char *s)
>>> return json_string_create_nocopy(xstrdup(s));
>>> }
>>>
>>> -struct json *
>>> -json_serialized_object_create(const struct json *src)
>>> +static struct json *
>>> +json_serialized_object_create__(const struct json *src, int flags)
>>> {
>>> struct json *json = json_create(JSON_SERIALIZED_OBJECT);
>>> - json->string = json_to_string(src, JSSF_SORT);
>>> + json->string = json_to_string(src, flags);
>>> return json;
>>> }
>>>
>>> +struct json *
>>> +json_serialized_object_create(const struct json *src)
>>> +{
>>> + return json_serialized_object_create__(src, JSSF_SORT);
>>> +}
>>> +
>>> +struct json *
>>> +json_serialized_object_create_with_yield(const struct json *src)
>>> +{
>>> + return json_serialized_object_create__(src, JSSF_SORT | JSSF_YIELD);
>>> +}
>>> +
>>> struct json *
>>> json_array_create_empty(void)
>>> {
>>> @@ -360,20 +373,20 @@ json_integer(const struct json *json)
>>> return json->integer;
>>> }
>>>
>>> -static void json_destroy_object(struct shash *object);
>>> -static void json_destroy_array(struct json_array *array);
>>> +static void json_destroy_object(struct shash *object, bool yield);
>>> +static void json_destroy_array(struct json_array *array, bool yield);
>>>
>>> /* Frees 'json' and everything it points to, recursively. */
>>> void
>>> -json_destroy__(struct json *json)
>>> +json_destroy__(struct json *json, bool yield)
>>> {
>>> switch (json->type) {
>>> case JSON_OBJECT:
>>> - json_destroy_object(json->object);
>>> + json_destroy_object(json->object, yield);
>>> break;
>>>
>>> case JSON_ARRAY:
>>> - json_destroy_array(&json->array);
>>> + json_destroy_array(&json->array, yield);
>>> break;
>>>
>>> case JSON_STRING:
>>> @@ -395,13 +408,16 @@ json_destroy__(struct json *json)
>>> }
>>>
>>> static void
>>> -json_destroy_object(struct shash *object)
>>> +json_destroy_object(struct shash *object, bool yield)
>>> {
>>> struct shash_node *node;
>>>
>>> SHASH_FOR_EACH_SAFE (node, object) {
>>> struct json *value = node->data;
>>>
>>> + if (yield) {
>>> + cooperative_multitasking_yield();
>>> + }
>>
>> Should this be moved to the top of the function?
>>
>>> json_destroy(value);
>>
>> Should be a conditional call to with_yield() version as well?
>> Inner objects can be huge.
>>
>> AFAIU the code here, only objects yield. Other types of JSON
>> do not yield. And that is fine. However, if the object only
>> contains simple elements, yielding on each of them is likely
>> unnecessary. If elements of the object are objects themselves,
>> then they will yield.
>>
>> For example, in case of a JSON-formatted database, we will yield
>> once per row in this case. IIUC, current patch will yield once
>> per table, because an update is an array of table objects that
>> contain row objects.
>>
>> What do you think?
>>
>>> shash_delete(object, node);
>>> }
>>> @@ -410,12 +426,13 @@ json_destroy_object(struct shash *object)
>>> }
>>>
>>> static void
>>> -json_destroy_array(struct json_array *array)
>>> +json_destroy_array(struct json_array *array, bool yield)
>>> {
>>> size_t i;
>>>
>>> for (i = 0; i < array->n; i++) {
>>> - json_destroy(array->elems[i]);
>>> + yield ? json_destroy_with_yield(array->elems[i]) :
>>> + json_destroy(array->elems[i]);
>>
>> Please, use an if statement instead.
>>
>>> }
>>> free(array->elems);
>>> }
>>> @@ -1525,7 +1542,7 @@ static void json_serialize_object(const struct shash
>>> *object,
>>> struct json_serializer *);
>>> static void json_serialize_array(const struct json_array *,
>>> struct json_serializer *);
>>> -static void json_serialize_string(const char *, struct ds *);
>>> +static void json_serialize_string(const char *, struct json_serializer *);
>>>
>>> /* Converts 'json' to a string in JSON format, encoded in UTF-8, and
>>> returns
>>> * that string. The caller is responsible for freeing the returned string,
>>> @@ -1598,7 +1615,7 @@ json_serialize(const struct json *json, struct
>>> json_serializer *s)
>>> break;
>>>
>>> case JSON_STRING:
>>> - json_serialize_string(json->string, ds);
>>> + json_serialize_string(json->string, s);
>>> break;
>>>
>>> case JSON_SERIALIZED_OBJECT:
>>> @@ -1631,7 +1648,7 @@ json_serialize_object_member(size_t i, const struct
>>> shash_node *node,
>>> indent_line(s);
>>> }
>>>
>>> - json_serialize_string(node->name, ds);
>>> + json_serialize_string(node->name, s);
>>> ds_put_char(ds, ':');
>>> if (s->flags & JSSF_PRETTY) {
>>> ds_put_char(ds, ' ');
>>> @@ -1734,7 +1751,7 @@ static const char *chars_escaping[256] = {
>>> };
>>>
>>> static void
>>> -json_serialize_string(const char *string, struct ds *ds)
>>> +json_serialize_string(const char *string, struct json_serializer *s)
>>> {
>>> uint8_t c;
>>> uint8_t c2;
>>> @@ -1742,26 +1759,32 @@ json_serialize_string(const char *string, struct ds
>>> *ds)
>>> const char *escape;
>>> const char *start;
>>>
>>> - ds_put_char(ds, '"');
>>> + ds_put_char(s->ds, '"');
>>> count = 0;
>>> start = string;
>>> while ((c = *string++) != '\0') {
>>> + if (s->flags & JSSF_YIELD) {
>>> + cooperative_multitasking_yield();
>>> + }
>>
>> This looks like an overkill. Here we're yielding per character
>> in every JSON string. That's a lot.
>> Can we yield per object like it's done for a destruction instead?
>
> Looking at this again, it looks like I have mixed up the code paths of
> the type JSON_SERIALIZED_OBJECT with JSON_STRING so this is most
> likely not needed.
>
> That also explains one overrun source I was not able to identify
> before running out of time to send the initial patch set for review,
> sorry about that!
>
> We most likely need a chunked version of ds_put_cstr() instead though,
> as it is what most likely steals the time instead [0]. Will get back
> as soon as I've verified.
I'm not convinced this is a problem. Pre-serialized objects are
copied with a single memcpy() call. It should be fast enough even
if the string is 100MB long. I'm more concerned about not yielding
while serializing objects that have large sub-objects.
>
> 0:
> https://github.com/openvswitch/ovs/blob/60457a5e9ddc33809139e91b08634eacd766abb2/lib/json.c#L1605
>
> --
> Frode Nordahl
>
>>> if (c >= ' ' && c != '"' && c != '\\') {
>>> count++;
>>> } else {
>>> if (count) {
>>> - ds_put_buffer(ds, start, count);
>>> + ds_put_buffer(s->ds, start, count);
>>> count = 0;
>>> }
>>> start = string;
>>> escape = chars_escaping[c];
>>> while ((c2 = *escape++) != '\0') {
>>> - ds_put_char(ds, c2);
>>> + if (s->flags & JSSF_YIELD) {
>>> + cooperative_multitasking_yield();
>>> + }
>>> + ds_put_char(s->ds, c2);
>>> }
>>> }
>>> }
>>> if (count) {
>>> - ds_put_buffer(ds, start, count);
>>> + ds_put_buffer(s->ds, start, count);
>>> }
>>> - ds_put_char(ds, '"');
>>> + ds_put_char(s->ds, '"');
>>> }
>>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
