From 54b33d2184ab9fa8e6595a77ed5208bd0a4d2602 Mon Sep 17 00:00:00 2001 From: Abhinay Reddyreddy Date: Wed, 10 Oct 2018 08:16:13 -0400 Subject: [PATCH 1/5] RFC initial commit --- ...0-atcompoundkey-error-path-optimization.md | 201 ++++++++++++++++++ 1 file changed, 201 insertions(+) create mode 100644 text/0000-atcompoundkey-error-path-optimization.md diff --git a/text/0000-atcompoundkey-error-path-optimization.md b/text/0000-atcompoundkey-error-path-optimization.md new file mode 100644 index 0000000..9d21774 --- /dev/null +++ b/text/0000-atcompoundkey-error-path-optimization.md @@ -0,0 +1,201 @@ +- Start Date: 2018-10-05 +- RFC PR: +- CppMicroServices Issue: https://github.com/CppMicroServices/CppMicroServices/issues/302 + +# Improving the error path performance for AtCompoundkey API + +## 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. + +## 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. + +The following graph shows the difference between the performance of happy path and error path in AtCompoundKey API + +
+ Plot 1 + +
+ + +## Option 1 - Modify existing API to return a empty const Any& if the key is not found +Remove the non-const version of the API and retain just the const version. Return empty `Any` by reference if the key is not found + +Any& AtCompoundkey(const std::string& key); + +`const Any& AtCompoundKey(const std:;string&key) const;` + +### Pros +* Error path is much improved, almost equal to the happy path if not better + +### 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. +* The information regarding the error conditions is lost. However, do the users really care about the information is a puzzle. +* If the BundleManifest class is modified to handle "null" values in the manifest, this API has to change. + +## Option 1.1 - Add an overload with a default value parameter +Add an overload with the following signature: + +``` +const Any& AtCompoundKey(const std::string& key, const Any& defaultvalue) const; +``` + +Return the user provided default value if the key is not found in the map. + +### Pros +* Addresses the performance problem. The happy path and the error path have similar performance. +* Happy path performance does not degrade. +* Backwards compatible. Since this API has different number of arguments, this API can be added as an overload set. This allows us to let users choose the right API based on their performance and error handling requirements. Also, lets us deprecate the old one if no use-cases exist. + +### Cons +* User has to provide an extra argument for the default value. +* The information regarding the error conditions is lost. However, do the users really care about the information is a puzzle. +* The default value passed to the API must not be an inlined temporary since we return it by reference. + +## Option 2 - Modify the current API to return an Any object by value +Return an empty `Any` object if the key is not found + +``` +Any AtCompoundKey(const std::string& key); +``` + +### Pros +* Addresses the performance problem. The happy path and the error path have similar performance. +* Happy path performance does not degrade + +###Cons +* Not backwards compatible. +* Returning by value results in copies of the returned object which could be costly, specially if this API is used to retrieve intermediate maps of larger size. +* 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 he error conditions is lost. However, do the users really care about the information is a puzzle. + +## Option 3 - Expose the bundle's metadata as a third party JSON object. +Remove the `AtCompoundKey` API and expose the bundle's metadata in the form of an object of a JSON parsing library. E.g, rapidjson::Document from the RapidJson library + +``` +rapidjson::Document Bundle::GetHeaders(); +``` + +### Pros +* 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. +* No user data to support this strategy. We do not know if users would appreciate this option. + +## Option 4 + +Add a "KeyExists" API which returns a bool to signify the existence of the entry. + +``` +if(map.KeyExists("com.foo.bar")) +{ + auto val = map.AtCompoundKey("com.foo.bar"); +} +``` + +### Pros +* Similar to the STL map's "count" API +* Backwards compatible + +### Cons +* Users have to call this API before calling the existing AtCompoundKey API. The resulting code would require parsing the dot notation key twice which may not be desirable. + + +## Performance Comparison of Options + +Here's a graph showing the performance of various options proposed above + +
+ Plot 3 + +
+--- +
+ Plot 5 + +
+### Manifest file used for gathering the above metrics. +``` +{ + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": { + "relativelylongkeyname_element": true, + "relativelylongkeyname_map": {} + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } + } +} +``` + +## Unresolved questions + +> Will we ever support null values in bundle's manifest.json file? From 07f283aa714b09876fd3825c440a3eb30bf1f50e Mon Sep 17 00:00:00 2001 From: Abhinay Reddyreddy Date: Wed, 10 Oct 2018 08:26:40 -0400 Subject: [PATCH 2/5] changes based on suggestions from client team at mathworks Signed-off-by: Roy.Lurie@mathworks.com --- ...0-atcompoundkey-error-path-optimization.md | 42 +++++++++---------- 1 file changed, 19 insertions(+), 23 deletions(-) diff --git a/text/0000-atcompoundkey-error-path-optimization.md b/text/0000-atcompoundkey-error-path-optimization.md index 9d21774..84ca840 100644 --- a/text/0000-atcompoundkey-error-path-optimization.md +++ b/text/0000-atcompoundkey-error-path-optimization.md @@ -25,10 +25,10 @@ Remove the non-const version of the API and retain just the const version. Retur Any& AtCompoundkey(const std::string& key); -`const Any& AtCompoundKey(const std:;string&key) const;` +`const Any& AtCompoundKey(const std::string& key) const;` ### Pros -* Error path is much improved, almost equal to the happy path if not better +* Addresses the performance problem. The happy path and the error path have similar performance. ### Cons * Happy path has a slight performance decrease due to the use of 'find' API instead of the 'at' API @@ -36,41 +36,37 @@ Remove the non-const version of the API and retain just the const version. Retur * The information regarding the error conditions is lost. However, do the users really care about the information is a puzzle. * If the BundleManifest class is modified to handle "null" values in the manifest, this API has to change. -## Option 1.1 - Add an overload with a default value parameter -Add an overload with the following signature: +## Option 2 - Modify the current API to return an Any object by value +Return an empty `Any` object if the key is not found ``` -const Any& AtCompoundKey(const std::string& key, const Any& defaultvalue) const; +Any AtCompoundKey(const std::string& key); ``` -Return the user provided default value if the key is not found in the map. - ### Pros * Addresses the performance problem. The happy path and the error path have similar performance. -* Happy path performance does not degrade. -* Backwards compatible. Since this API has different number of arguments, this API can be added as an overload set. This allows us to let users choose the right API based on their performance and error handling requirements. Also, lets us deprecate the old one if no use-cases exist. -### Cons -* User has to provide an extra argument for the default value. -* The information regarding the error conditions is lost. However, do the users really care about the information is a puzzle. -* The default value passed to the API must not be an inlined temporary since we return it by reference. +###Cons +* Returning by value results in copies of the returned object which could be costly, specially if this API is used to retrieve intermediate maps of larger size. +* 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 he error conditions is lost. However, do the users really care about the information is a puzzle. -## Option 2 - Modify the current API to return an Any object by value -Return an empty `Any` object if the key is not found +## Option 2.1 (Preferred) - Add an overload with a default value parameter +Add an overload with the following signature: ``` -Any AtCompoundKey(const std::string& key); +Any AtCompoundKey(const std::string& key, Any&& defaultvalue) const; ``` +Return the user provided default value if the key is not found in the map. + ### Pros * Addresses the performance problem. The happy path and the error path have similar performance. -* Happy path performance does not degrade +* Backwards compatible. Since this API has different number of arguments, this API can be added as an overload set. This allows us to let users choose the right API based on their performance and error handling requirements. Also, lets us deprecate the old one if no use-cases exist. -###Cons -* Not backwards compatible. -* Returning by value results in copies of the returned object which could be costly, specially if this API is used to retrieve intermediate maps of larger size. -* 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 he error conditions is lost. However, do the users really care about the information is a puzzle. +### Cons +* User has to provide an extra argument for the default value. +* The information regarding the error conditions is lost. However, do the users really care about the information is a puzzle. ## Option 3 - Expose the bundle's metadata as a third party JSON object. Remove the `AtCompoundKey` API and expose the bundle's metadata in the form of an object of a JSON parsing library. E.g, rapidjson::Document from the RapidJson library @@ -84,7 +80,7 @@ rapidjson::Document Bundle::GetHeaders(); ###Cons * Users have to parse through the JSON tree to retrieve the values. -* No user data to support this strategy. We do not know if users would appreciate this option. +* 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 ## Option 4 From 2ad202cf6aac173ad778643bfc85d9c552b3dc9c Mon Sep 17 00:00:00 2001 From: Abhinay Reddyreddy Date: Wed, 10 Oct 2018 11:10:02 -0400 Subject: [PATCH 3/5] Fixed a typo and changed the function signature --- text/0000-atcompoundkey-error-path-optimization.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/text/0000-atcompoundkey-error-path-optimization.md b/text/0000-atcompoundkey-error-path-optimization.md index 84ca840..248eae4 100644 --- a/text/0000-atcompoundkey-error-path-optimization.md +++ b/text/0000-atcompoundkey-error-path-optimization.md @@ -6,11 +6,11 @@ ## 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. +Using AtCompoundKey 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. ## 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. +AnyMap provides a convenience 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. The following graph shows the difference between the performance of happy path and error path in AtCompoundKey API @@ -55,7 +55,7 @@ Any AtCompoundKey(const std::string& key); Add an overload with the following signature: ``` -Any AtCompoundKey(const std::string& key, Any&& defaultvalue) const; +Any AtCompoundKey(const std::string& key, Any defaultvalue) const; ``` Return the user provided default value if the key is not found in the map. @@ -66,7 +66,7 @@ Return the user provided default value if the key is not found in the map. ### Cons * User has to provide an extra argument for the default value. -* The information regarding the error conditions is lost. However, do the users really care about the information is a puzzle. +* The information regarding the error conditions is lost. However, if the users care about the error conditions, they can use the overload version that throws. ## Option 3 - Expose the bundle's metadata as a third party JSON object. Remove the `AtCompoundKey` API and expose the bundle's metadata in the form of an object of a JSON parsing library. E.g, rapidjson::Document from the RapidJson library From dc7c2cc3de9d6fe7eff667ae41d2838097d508c1 Mon Sep 17 00:00:00 2001 From: Abhinay Reddyreddy Date: Wed, 10 Oct 2018 11:13:49 -0400 Subject: [PATCH 4/5] fixed typo Signed-off-by: The Mathworks Inc --- text/0000-atcompoundkey-error-path-optimization.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/text/0000-atcompoundkey-error-path-optimization.md b/text/0000-atcompoundkey-error-path-optimization.md index 248eae4..086b2ac 100644 --- a/text/0000-atcompoundkey-error-path-optimization.md +++ b/text/0000-atcompoundkey-error-path-optimization.md @@ -6,7 +6,7 @@ ## Motivation -Using AtCompoundKey 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. +Using AtCompoundKey with non-existent keys produces an exception. If the API is used extensively, the application performace is degraded as reported by a user. Users prefer not to pay for the cost of exception handling in case a compound key is not found in the AnyMap. ## Description of the Problem @@ -78,7 +78,7 @@ rapidjson::Document Bundle::GetHeaders(); ### Pros * Users can use the JSON object to write their custom parsing functions or pass around JSON objects in their code -###Cons +### 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 From c1986437bc747894fa2119c38f71e7df48e05603 Mon Sep 17 00:00:00 2001 From: Abhinay Reddyreddy Date: Thu, 11 Oct 2018 09:41:42 -0400 Subject: [PATCH 5/5] Fixes as per feedback Signed-off-by: The Mathworks Inc --- text/0000-atcompoundkey-error-path-optimization.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/text/0000-atcompoundkey-error-path-optimization.md b/text/0000-atcompoundkey-error-path-optimization.md index 086b2ac..ca58aa1 100644 --- a/text/0000-atcompoundkey-error-path-optimization.md +++ b/text/0000-atcompoundkey-error-path-optimization.md @@ -6,11 +6,13 @@ ## Motivation -Using AtCompoundKey with non-existent keys produces an exception. If the API is used extensively, the application performace is degraded as reported by a user. Users prefer not to pay for the cost of exception handling in case a compound key is not found in the AnyMap. +Using AtCompoundKey with non-existent keys produces an exception. If the API is used extensively (in a tight loop or called multiple times), the application performace is degraded as reported by a user. Users prefer not to pay for the cost of exception handling in case a compound key is not found in the AnyMap. + +Consider the use case where a user has thousands of bundles with custom metadata. The metadata in all the bundles is not uniform. i.e, some bundles may have some properties while others don't. If the AtCompoundKey API is applied for each of the bundles in the installed bundle list, there are a lot of exceptions raised from the API. The user code has to handle the exceptions which turns out to be expensive. ## Description of the Problem -AnyMap provides a convenience 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. +AnyMap provides a convenience 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 has resulted in performace degradation in some use cases. The performace hit is due to the 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. The following graph shows the difference between the performance of happy path and error path in AtCompoundKey API @@ -46,10 +48,10 @@ Any AtCompoundKey(const std::string& key); ### Pros * Addresses the performance problem. The happy path and the error path have similar performance. -###Cons +### Cons * Returning by value results in copies of the returned object which could be costly, specially if this API is used to retrieve intermediate maps of larger size. * 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 he error conditions is lost. However, do the users really care about the information is a puzzle. +* The information regarding the error conditions is lost. ## Option 2.1 (Preferred) - Add an overload with a default value parameter Add an overload with the following signature: @@ -65,7 +67,6 @@ Return the user provided default value if the key is not found in the map. * Backwards compatible. Since this API has different number of arguments, this API can be added as an overload set. This allows us to let users choose the right API based on their performance and error handling requirements. Also, lets us deprecate the old one if no use-cases exist. ### Cons -* User has to provide an extra argument for the default value. * The information regarding the error conditions is lost. However, if the users care about the error conditions, they can use the overload version that throws. ## Option 3 - Expose the bundle's metadata as a third party JSON object. @@ -98,7 +99,8 @@ if(map.KeyExists("com.foo.bar")) * Backwards compatible ### Cons -* Users have to call this API before calling the existing AtCompoundKey API. The resulting code would require parsing the dot notation key twice which may not be desirable. +* The extra API call (to KeyExists) clients need to make, which is not as simple as having to make one API call. +* Possible performance hit from parsing a string and traversing the map twice. ## Performance Comparison of Options