Unify json serialization by using rapidjson #2960

aarranz · 2017-07-20T12:00:32Z

Current implementation of the Context Broker has a mixture of code for serializing JSON structures. I'm proposing to change it to use rapidjson.

Current pull request code is confirmed to fix bug #2955 and it's passing all the unit test (including a new test for stressing bug #2955). I'm currently passing the valgrind tests (at least 0000_https_support/https and 0000_ipv6_support/ipv4_ipv6_both are failing) so probably this pull request has to be updated.

Anyway, it also simplifies current context broker code by removing the need to deal with JSON commas, different function for serializing JSON, ...

It probably fix (or can easily be extended to fix) bugs #2938, #2425, #2959 as well as other bugs regarding JSON serialization.

…json

…on structures

…h updating/passing the tests

fgalan · 2017-07-20T15:58:53Z

Impressive work... +/- 6000 changed lines ;)

One quick question, before analyzing the work and providing more in-deep feedback. You say:

I'm currently passing the valgrind tests [...]

Do you mean valgrind suite (i.e. make valgrind) or functional test (i.e. make ft) ?

aarranz · 2017-07-20T16:53:17Z

I mean make valgrind and it's taking a lot of time (not finished yet, currently running test 440). I know that there are functional tests not passed when using the make valgrind command. I will run make ft after this first round using make valgrind.

These are the reported errors until now:

Test 023/927: 0000_https_support/https .............................................................................................................. OK (15.10 seconds) (no leak but ftest error 11)
Test 024/927: 0000_ipv6_support/ipv4_ipv6_both ...................................................................................................... OK (76.73 seconds) (no leak but ftest error 1)
Test 144/927: 0000_statistics_operation/statistics_with_semaphores .................................................................................. OK (55.44 seconds) (no leak but ftest error 1)
Test 369/927: 0876_attribute_dates/filter_attr_datecreated_sub ...................................................................................... OK (91.25 seconds) (no leak but ftest error 1)
Test 370/927: 0876_attribute_dates/filter_attr_datemodified_sub ..................................................................................... OK (82.35 seconds) (no leak but ftest error 1)

fgalan · 2017-07-20T17:05:55Z

The valgrind regression is time intensive. As figure, it could take around ~~2-3 hours~~ 5-6 hours in our reference CI platform.

For the future, do make ft first. The valgring regression is typically done after all the functional test (and unit test) are ok so the only thing that is pending is to check about memory leaks and memory access errors.

fgalan · 2017-07-25T14:57:28Z

It probably fix (or can easily be extended to fix) bugs #2938, #2425, #2959 as well as other bugs regarding JSON serialization.

I think the reference to #2959 is a typo. Actually #2959 is a fix in the valgrind test framework...

fgalan · 2017-07-26T07:36:10Z

Finally, we have had some time to have a look to this PR a bit closer.

First, thanks for your contribution! In some sense, it addresses the same old technical debt topic related with improving responses payload rendering described in #1298. Moreover, the PR has adopted a smarter approach, removing some structures (iostreams) that raised concern in some developers about performance in #1298. In addition, the removal of many internal returning of std::string between render methods (in favor of JsonHelper/writer reference passing) is also a good point.

Let’s consider the two different families in the NGSI API, i.e. NGSIv1 and NGSIv2.

Regarding NGSIv2, I understand this PR doesn’t change anything from a functional point of view. In other words, all the NGSIv2 .test should pass regression ok, even concerning Content-Length in response payload. However, a rendering-intensive performance test is required, in order to see that performance is not decreasing (or even better, is increasing :)

Regarding NGSIv1, it is a bit more complicated… The work in this PR collides with the payload simplification work already started at haderding/remove_ngsiv1_indent branch (yes, branch name has a typo ;). Although probably the work done in the present PR would reach at the end a similar result to the one in that branch, we have paid an extreme care to backward compatibility in it, including the “NGSIv1 indent paranoid” CLI flag, that we don’t want to lose. In fact, most probably current PR is not passing NGSIv1 .tests as they are now. Have a look to #2760 for more detail on haderding/remove_ngsiv1_indent work.

Thus, our proposal is to reduce the scope of this PR (maybe in a new fresh PR, leaving the present one to be closed), limiting to NGSIv2 renders by the moment. Once you have done that, let’s ensure that: 1) all .test (both NGSIv1 and NGSIv2) pass without changes, 2) the result of an NGSIv2 rendering-intensive performance test is positive (that test have to be carefully designed), 3) there is not conflict with haderding/remove_ngsiv1_indent work.

In a second iteration, once haderding/remove_ngsiv1_indent gets consolidated into master, the same approach (i.e. using rapidjson to render NGSIv1 responses) could be done. But that’s a separate piece of work for the future.

In addition to the above, the PR has some code style issues (see contribution guidelines for more detail). @kzangeli could elaborate on that… he knows more on style than me :)

One final note… the PR is HUGE. It is very difficult to review it in deep. In fact, we are not sure of having capture all the valuable feedback or missing potential problems with it :) Reducing the scope to just NGSIv2 could be also a good idea to make the review task easier. Moreover, maybe is a good idea to start with a first little PR addressing the NGSIv2 (covering just a small subset of render methods, corresponding to just one NGSIv2 operation) and progressing step by step in successive easy to review PRs.

fgalan · 2017-07-26T07:38:01Z

src/lib/common/dtoa.h

@@ -0,0 +1,239 @@
+/*


This file is already part of radidjson (typically, /usr/local/include/rapidjson/internal/dtoa.h). Maybe it could be used from the library instead of including it as Orion library?

I have included this file to change how rapidjson render numbers (I am working now on creating a PR in rapidjson). I think is a bug to use a different syntax when rendering float and integer numbers. Take into account that JSON only uses one type for numbers (numbers are always floating numbers, there is not a different syntax for integer numbers).

The rendering problem is exposed when rendering an integer number. E.g. if you use the Double method provided by rapidjson to render a 4 number it is rendered as: 4.0. This syntax is required by some languages (like C/C++) to mark it as a floating number, but in JSON it is not required, so it would be better written as 4, and is what is usually done by other libraries (in fact, rapidjson is the only library I know that serialises numbers using this C/C++ pattern)

kzangeli · 2017-07-26T08:10:09Z

src/lib/common/JsonHelper.cpp

 }


+void JsonHelper::Null() {


First of all, great job!

A minor detail, this line here (and many lines that follow), doesn't follow the Orion style guide (see https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/contribution_guidelines.md).

As you will see in that doc, and throughout the code, we put brackets ({}) on a line of its own, all functions have a function header and functions/methods are camelCase, starting with lowercase (null, int, etc may give problems with the compiler for being reserved words ... - if so, an exception-decision needs to be made, and added to the style guide).

Please read the style guide and fix these minor details.
[ I wont add a github-comment for each occurrence here, but instead do a re-check after this has been fixed ]

ok, I will fix those problems.

kzangeli · 2017-07-26T08:13:42Z

src/lib/common/JsonHelper.cpp

+  if (indent == 0) {
+    writer.Bool(value);
+  } else {
+    pwriter.Bool(value);


Not sure it is possible, but it would be great to replace all these if (indent == 0) { writer } else { pwriter } for a single if upon creation of the writer/pwriter object. Polymorhphism ... (this wont change during the rendering of one JSON object - it's either pretty or not). If this can be done, the code would gain a lot in readability and number of source code lines.

Also, a doubt about writer/pwriter.
This code now seems to be prepared to either do "pretty printing" or "no whitespace".
How/where is the mode selected?

It would be great (personal opinion) to have a URI parameter like ?output=formated or something similar to let the client select. This is something we've been thinking about for a long time ...

However, not to be included in this very PR, it is very big already.

I also hate those if sentences. The problem is that polymorhphism cannot be used in this case due rapidjson providing the Writer and PrettyWriter classes using class templates. Moreover, currently, rapidjson is not marking the required methods as virtual (and it is very difficult to use that scheme using class templates, due how class templates are implemented in C++).

Regarding supporting the ?output=formated seems easy to implement. But I also agree with you regarding implementing it after cleaning and merging this PR.

@aarranz remember we have a pending talk. Don't continue coding on this PR without talking with me first ;)

fgalan · 2018-12-03T12:32:08Z

This PR cannot be considered stale. It shows how rapidjson can be used to render JSON payload and, in that sense, it is useful.

aarranz added 7 commits July 20, 2017 12:57

Replace the use of the tag macros and functions by the usage of rapid…

912114d

…json

Remove the now unused tag module

d8a2fbb

Refactor: render method for building payloads, toJson for building js…

5fb27be

…on structures

Fixed some problems with pretty printing jsons and fine tuning throug…

b6185ef

…h updating/passing the tests

Add a new unit test stressing json serialization

7e7cfad

Update changelog

03dc6fa

Fix JSON number serialization

ce20145

fgalan reviewed Jul 26, 2017

View reviewed changes

kzangeli suggested changes Jul 26, 2017

View reviewed changes

fgalan mentioned this pull request Jun 28, 2018

JsonHelper usage in all NGSIv2 renders (second step: optimization and performance) #1298

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify json serialization by using rapidjson #2960

Unify json serialization by using rapidjson #2960

aarranz commented Jul 20, 2017 •

edited

Loading

fgalan commented Jul 20, 2017

aarranz commented Jul 20, 2017

fgalan commented Jul 20, 2017 •

edited

Loading

fgalan commented Jul 25, 2017

fgalan commented Jul 26, 2017

fgalan Jul 26, 2017

aarranz Sep 1, 2017

aarranz Sep 1, 2017

kzangeli Jul 26, 2017

aarranz Sep 1, 2017

kzangeli Jul 26, 2017

kzangeli Jul 26, 2017 •

edited

Loading

aarranz Sep 1, 2017

fgalan Sep 1, 2017

fgalan commented Dec 3, 2018

		}


		void JsonHelper::Null() {

Unify json serialization by using rapidjson #2960

Are you sure you want to change the base?

Unify json serialization by using rapidjson #2960

Conversation

aarranz commented Jul 20, 2017 • edited Loading

fgalan commented Jul 20, 2017

aarranz commented Jul 20, 2017

fgalan commented Jul 20, 2017 • edited Loading

fgalan commented Jul 25, 2017

fgalan commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan commented Dec 3, 2018

aarranz commented Jul 20, 2017 •

edited

Loading

fgalan commented Jul 20, 2017 •

edited

Loading

kzangeli Jul 26, 2017 •

edited

Loading