Introduce new JSON parser utility

[wenduwan]

This patch introduces a utility in OPAL based on the 3rd-party project https://github.com/json-parser/json-parser.git

The utility provides APIs to read JSON into memory along with getters to retrieve C values.

Please see the unit test opal_json.c for example usage.

[lrbison]

I understand the desire to insulate ourselves from the specific json reader implementation by abstracting out an interface for us to use. However, I am a little uncomfortable inventing a new interface for a json reader. Relevant XKCD. Why not use json-parser's json.c/json.h symbols directly?

[wenduwan]

@lrbison For the most part, json-parser only provides an API to parse a string. This PR extended that API to support filename.

Another reason is that I modeled the JSON object with a opal_object_t subclass. The data structure is not compatible with json-parser's json_value, so we need a layer to hide the latter.

[juntangc]

It's probably better to add some descriptions on how to use the json parser than pointing the users to some source code. Please also include the new features added and how to convert existing tuning files into the json files.

[wenduwan]

That's a good idea. I can add an example later.

[bosilca]

Looks good, but there are still some questions pending.

1. Can you don't use the same name for the two .[ch] files. I know they are in different directories, but the identical name is confusing.
2. What is the benefit to have the OPAL_OBJECT support ? Is this benefit worth enough to account for the extra memory and CPU needed (including the refcount) to handle the opal_object_t.

[wenduwan]

@bosilca Thank you very much for the review. I have renamed the files as suggested.

What is the benefit to have the OPAL_OBJECT support ? Is this benefit worth enough to account for the extra memory and CPU needed (including the refcount) to handle the opal_object_t.

The primary motivation is memory management. In json-parser, only the root object should be freed after use - the child objects are merely pointers to the root. This can be tricky to use. With the opal object lifecycle hooks we can override the destructor to correctly free the root object only, while the application can safely OBJ_RELEASE the objects in any order. I understand that this can also be achieved with an explicit destructor function opal_json_free but I like the OPAL_OBJ pattern.

TBH I am not concerned with resource usage - the parser should only be used to read the file into memory and stored somewhere else, e.g. a schema with efficient query methods. Afterwards the parser should be torn down.

[wenduwan]

@juntangc I added an example program in opal_json.h. Please take a look.

[bosilca]

If you don't add all the OPAL objectification you only have to free a single object, the initial json object via json_value_free. Simpler and cleaner, no need for extra reference counts, nor extra memory allocations. JSON manipulation is simple and straightforward, we should try to keep it that way.

[wenduwan]

If you don't add all the OPAL objectification you only have to free a single object, the initial json object via json_value_free. Simpler and cleaner, no need for extra reference counts, nor extra memory allocations. JSON manipulation is simple and straightforward, we should try to keep it that way.

That is also my goal. But my opinion differs in that:

1. I would like to be more defensive against double free behaviors. Instead of trusting the user to correctly remember the root object and NOT free its children by accident, we can provide a better user experience by hiding this detail and handle it internally.
2. IMO the OPAL_OBJECT provides a consistent user interface and uniform coding style. I imagine this is also lowers learning curve for the user.

[bosilca]

To state this differently instead of trusting to user to release a single object once (aka. the main json object she obtained from the load function), you trust them to release each object resulting from opal_json_get_key, opal_json_get_index in addition to the high level object obtained via opal_json_load*. Sorry, but I'm hardly convinced.

[bosilca]

I think we can solve our disagreement if instead of opal_json_t** the json API takes an opal_json_t*object. This way the user will always be in charge of the life expectancy of the objects, and the code will now really look as in the example (the one where the upper level object is overwritten by the lower json reader).

[wenduwan]

@bosilca Thinking more about OPAL_OBJECT I have a question about inheritance:

In this PR I want to hide json_value from a 3rd-party library and only expose

struct opal_json_t {
    opal_object_t super;
    opal_json_type type;
};

Currently I take advantage of the subclass

struct opal_json_internal_t {
    struct opal_json_t parent;
    json_value *value;
};

This makes the intention clear that the user should not touch the internals beyond struct opal_json_t.

I wonder what the alternative is if I don't use OPAL_OBJECT. It is obvious to me that we can still do something similar without subclassing, e.g.

struct opal_json_t {
    opal_json_type type;
};
...
struct opal_json_internal_t {
    struct opal_json_t parent;
    json_value *value;
};

Would this be better in your opinion?

[wenduwan]

@lrbison @bosilca I added a commit that show the difference if I don't use OPAL_OBJECT. Please take a look and let me know if you like it better.

[bosilca]

looks much better to me.

[jsquyres]

Shouldn't this be a show_help, not a direct opal_output_verbose? (same goes for all cases of errors emitted via opal_output_verbose)

[wenduwan]

Thanks. I switched to show help.

[jsquyres]

Is json set to NULL upon return? The param is marked in, but you pass a **.

[wenduwan]

Nope json is not modified upon return.

The ** is due to the fact of that json being used(and likely declared) as const * everywhere - here we need a double pointer to tell the compiler that we are not modifying json, but rather a pointer to it.

[jsquyres]

Is this returning the number of elements in the json object, or the size of the object in bytes?

The name is a little misleading -- IMHO, size refers to number of bytes. Could we use opal_json_get_num_keys or somesuch?

[bosilca]

An object is untyped, it cannot contain elements, it only contains bytes. The other function you are referring to is opal_json_get_array_len but it can only apply to arrays.

[wenduwan]

@bosilca Actually this function is also counting object length, not bytes.

I wonder if I should simply keep 1 function for both object and array, i.e. opal_json_get_len.

[bosilca]

got it, object is a keymapped values storage of json_value_t* while array is just an array of json_value_t*. Then a single assessor would be nice.

[wenduwan]

Thanks. I decided to keep a single function opal_json_get_container_size for both array and object. They are too similar to have different functions.