-
Notifications
You must be signed in to change notification settings - Fork 266
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JsonHelper usage in all NGSIv2 renders (second step: optimization and performance) #1298
Comments
Before making this big change, that definitely has its advantages, we need to make sure the broker doesn't loose too much in terms of throughput. |
Actually, what we have to check is not only the change in throughput, but the trade off between throughput and code simplification (i.e. small loss in throughput is acceptable if the gain in code simplification is big). |
Yes, that is exactly what I said :-) |
With regards to payload rendering two actions needs to be done. A first action is related with unification and simplification. Due to the organic evolution of the code during years, we have ended with several ways of rendering payloads. We should have ONLY ONE pattern along all code. Having only one pattern will be a big gain in simplification and homogeneity and will allow to focus optimizations (related with the second action described in this comment, see below) in just one point. The pattern to use should be the one based in the JsonHelper.h API (used at the current code for instance in Subscription::toJson() or Registrations::toJson()). The approach of composing the document to render using “add” methods, then getting the string corresponding to the final composition is very powerful in terms of simplicity of use and frees the developer of dealing directly with start/ending tags, commas and all that delicate and distracting stuff. The second action is related with optimization and performance. JsonHelper implementation (JsonHelper.cpp) is based in an internal buffer, currently implemented using a std::ostringstream object. However, there are alternatives:
The different implementation alternatives would be internal modifications to JsonHelper.cpp and will not modify the JsonHelper.h API exported to other functions. During the optimitization work each implementation could live in a separated branch. This work will be fact-driven based in performance testing and comparison. Thus, metrics for the different implementation alternatives will be obtained based in the same scenario. That scenario should be one in which the payload rendering has an important impact, e.g. populate DB with 1000 large elements (entities or registrations), then GET them with
Running CB as:
with an index for |
Note that both actions are independent. I mean, the work can start extending the JsonHelper.h usage to all renders which are not currently using it. Or, on the contrary, the JsonHelper.cpp implementation can be optimized based on testing the payloads that currently are using it (registrations, for instance). |
With regards to unification and simplification action, NGSIv1 renders may be left apart and stay as they are now. NGSIv1 is semi-deprecated code, so effort invested on it is semi-wasted. |
This new bug #3280 raises the importance of progressing in this issue. Thus, as a first step I'd suggest a PR doing the following:
That PR should hopefully fix also #3280 |
PR #3285 addresses this issue (but probably more ones will follow). |
PR #3292 continues the simplification work. |
PR #3301 continues simplification work (unification of ContextElement and Entity classes) |
PR #3324 continues simplification work: removal os never implemented idDomain and reg metadata in NGSIv1 and renaming render() to toJsonV1(). |
PR #3329 continues simplifciation work (see PR body for details) |
Raised in PR #3329 (actually, raised in previously, but we have remember when discussing that PR :):
Maybe that object could be an instance of a rapidjson tree |
PR #3330 unified ContextElementVector into EntityVector |
PR #3336 ensures that old JSON_ macros are using only in the NGSIv1 renderint part. |
Fixes in devel manual done in PR #3339 |
…_cleanup REFACTOR related with #1298
Fix devel manual after changes implemented as part of #1298
(JP) Fix devel manual after changes implemented as part of #1298
With regards to the two steps described in this previous comment I'd say that at the present momento (3.7.0) the unification and simplification was completed time ago. Thus, the pending item of this work on this issue would be optimization and performance. I'll change issue title to properly align with this scope. |
JsonHelper class was introduced in PR #1284 as a way of simplifying the JSON response rendering (at that time only used in the JSON response for the "get all entities" operations). In addition, the class allows to rendering logic in a common place, which is interesting as it allows to solve problems such the one described in #1172 in a single point.
This issue is about extending the usage of JsonHelper to the other NGSIv2 types (and adapt the JsonHelper class in the process, if needed). As a result, we should see a reduction of the length and complexity of the current render() methods, as the start to use
addRaw()
,addString()
.Effort: 5 man day
The text was updated successfully, but these errors were encountered: