302 at compound key errorpath optimization #4
Conversation
|
|
||
| ## Motivation | ||
|
|
||
| Using AtCoumpoundKey with non-existent keys produces an exception. If the API is used extensively, the application performace is degraded as reported by a team at Mathworks Inc. Users prefer not to pay for the cost of exception handling in case a compound key is not found in the AnyMap. |
There was a problem hiding this comment.
AtCoumpoundKey -> AtCompoundKey
|
|
||
| ## Description of the Problem | ||
|
|
||
| AnyMap provides a convinience API to retrieve objects from the map using compound keys with dot notation. The API is very nifty and saves users from writing redundant code to parse through the nested maps. However, the performance of the error path is significantly higher than for the happy path in this API. This have resulted in performace degradation in some use cases. The performace hit is due to C++ exception handling mechanism. Although AtCompoundKey is similar to the STL map's `at` API, STL maps have other methods such as the `find` which can be used to efficiently access an element in the map without having to deal with the exceptions. |
| Add an overload with the following signature: | ||
|
|
||
| ``` | ||
| Any AtCompoundKey(const std::string& key, Any&& defaultvalue) const; |
There was a problem hiding this comment.
The issue here is that the clients can't create a default "sentinel" Any value and keep using it for further AtCompoundKey calls i.e. under this scenario, the following use case isn't possible
Any sentinel = std::string("sentinel");
auto anyval = myanymap.AtCompoundKey("foo.bar.baz", sentinel);
if (anyval == sentinel) {...} else {...}
auto anyval2 = myanymap.AtCompoundKey("foo.bar.doe", sentinel);
...The user is forced to do the following which maybe a pain-point
Any sentinel = std::string("sentinel");
auto anyval = myanymap.AtCompoundKey("foo.bar.baz", std::move(sentinel));
if (anyval == std::string("sentinel") {...} else {...}
Any sentinel2 = std::string("sentinel");
auto anyval2 = myanymap.AtCompoundKey("foo.bar.doe", std::move(sentinel2));- a solution is to simply pass the default by value?
- another is to have 2 overloads
- one with
const Any& default - delete the move variant i.e.
Any AtCompoundKey(const std::string& key, Any&& defaultvalue) const = delete;? (Note: only if we think the users won't prefer to pass temporaryAnys as a default value)
- one with
There was a problem hiding this comment.
pass by value seems simpler. Not sure if the users won't want to pass a temporary.
Signed-off-by: The Mathworks Inc <[email protected]>
Signed-off-by: The Mathworks Inc <[email protected]>
|
|
||
| ### Cons | ||
| * Happy path has a slight performance decrease due to the use of 'find' API instead of the 'at' API | ||
| * Although the API returns a const reference, the user could add back a non-const version by const_cast'ing the returned ref, which will break every subsequent call. Infact the current non-const API does exactly this, which is ikky. |
There was a problem hiding this comment.
I do not think that this is a real concern. const-casting will always be fishy. Another example is #define private public before including third-party headers and then accessing their private members. Just because users can do this, we do not stop using classes.
| * Users can use the JSON object to write their custom parsing functions or pass around JSON objects in their code | ||
|
|
||
| ### Cons | ||
| * Users have to parse through the JSON tree to retrieve the values. |
There was a problem hiding this comment.
I do not understand how this is substantially different from the AnyMap API. In JSON, there is actually a even better way of accessing elements in a hierarchy directly, RFC 6901. This is supported by at least rapidjson and nlohmann JSON.
There was a problem hiding this comment.
This is interesting. Thanks for the link, @saschazelzer!
|
|
||
| ### Cons | ||
| * Users have to parse through the JSON tree to retrieve the values. | ||
| * The team currently using AtCompoundKey does not like this option. They do not want to deal with the any_cast at each level of the map |
There was a problem hiding this comment.
It looks like the opinion is based on wrong assumptions or an incomplete picture of available JSON APIs. For example, another benefit of existing JSON libraries is their support for implicit conversions, which any_cast does not support.
There was a problem hiding this comment.
On a cursory look. rapidjson's GenericValue API has methods to query type, existence and extract value. Its not clear from the documentation what will happen if I call the GetArray method and the type is not an array. The implementation of the Get methods use RAPIDJSON_ASSERT which is a no-op in production. There is an option to override the MACRO to throw.
I will follow up with the client team about this option.
There was a problem hiding this comment.
Such conversions will typically throw or abort. I was rather referring to code like
int x = json["x"] or double x = json["x"] which would both work if "x" is convertible to int and double (rapidjson or nlohmann JSON).
|
|
||
| ## Unresolved questions | ||
|
|
||
| > Will we ever support null values in bundle's manifest.json file? |
There was a problem hiding this comment.
It doesn't look like it is important enough at this time.
| * It is not clear how a null value from a manifest file is treated. specifying "null" is legal in JSON. Current implementation of bundle metadata parser ignores the entries with null value. However, we cannot assume this will not change in the future. | ||
| * The information regarding the error conditions is lost. | ||
|
|
||
| ## Option 2.1 (Preferred) - Add an overload with a default value parameter |
There was a problem hiding this comment.
If we finally decide against the JSON API approach, I think a combination of 1. and 2.1. would make sense. Basically like the std::optional API and providing a const& Any AtCompoundKey(const std::string&) const as well as a Any AtCompoundKeyOr(const std::string&, Any) const function.
RFC for optimizing error path for AtCompoundKey, with design options and performance analysis.