Skip to content
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

Fast (and beautiful) json serializing #9832

Merged
merged 19 commits into from May 10, 2019
Merged

Conversation

elpaso
Copy link
Contributor

@elpaso elpaso commented Apr 18, 2019

Follow up from #9725

Now using this wonderful json lib:
https://github.com/nlohmann/json

Results in release mode, QJson tests are not shown but
QJson was even slower than string concat (this is the reason why I abandoned it).

PASS   : TestQgsJsonUtils::testExportAttributesJson(Use json)
RESULT : TestQgsJsonUtils::testExportAttributesJson():"Use json":
     0.0022 msecs per iteration (total: 75, iterations: 32768)
PASS   : TestQgsJsonUtils::testExportAttributesJson(Use old string concat)
RESULT : TestQgsJsonUtils::testExportAttributesJson():"Use old string concat":
     0.0032 msecs per iteration (total: 54, iterations: 16384)
PASS   : TestQgsJsonUtils::testExportFeatureJson(Use json)
RESULT : TestQgsJsonUtils::testExportFeatureJson():"Use json":
     0.011 msecs per iteration (total: 96, iterations: 8192)
PASS   : TestQgsJsonUtils::testExportFeatureJson(Use old string concat)
RESULT : TestQgsJsonUtils::testExportFeatureJson():"Use old string concat":
     0.015 msecs per iteration (total: 64, iterations: 4096)
PASS   : TestQgsJsonUtils::testExportGeomToJson(Use json)
RESULT : TestQgsJsonUtils::testExportGeomToJson():"Use json":
     0.76 msecs per iteration (total: 98, iterations: 128)
PASS   : TestQgsJsonUtils::testExportGeomToJson(Use old string concat)
RESULT : TestQgsJsonUtils::testExportGeomToJson():"Use old string concat":
     0.85 msecs per iteration (total: 55, iterations: 64)
PASS   : TestQgsJsonUtils::cleanupTestCase()

Note that there is an overhead in the benchmarks in converting to QString that will not be necessary in all scenarios, this means that this implementation will potentially be even more performant than in the above tests (QGIS server: I'm looking at you).

But speed is not everything: the json library offers a wonderful and easy to use API: https://github.com/nlohmann/json

I could spot a single small difference between the output generated by the new library and the old one and it is:

string to encode: "Some string with slashes / is encoded"
old: "Some string with slashes / is encoded"
new: "Some string with slashes \/ is encoded"

Note that the json standard allows to protect a forward slash but it does not enforce it.

Funded by: QCooperative.net

... you are too slow and QJson API is so ugly.

Now using this wonderful json lib:
https://github.com/nlohmann/json

Results in release mode (QJson tests are not shown but
QJson was even slower than string concat).

PASS   : TestQgsJsonUtils::testExportAttributesJson(Use json)
RESULT : TestQgsJsonUtils::testExportAttributesJson():"Use json":
     0.0022 msecs per iteration (total: 75, iterations: 32768)
PASS   : TestQgsJsonUtils::testExportAttributesJson(Use old string concat)
RESULT : TestQgsJsonUtils::testExportAttributesJson():"Use old string concat":
     0.0032 msecs per iteration (total: 54, iterations: 16384)
PASS   : TestQgsJsonUtils::testExportFeatureJson(Use json)
RESULT : TestQgsJsonUtils::testExportFeatureJson():"Use json":
     0.011 msecs per iteration (total: 96, iterations: 8192)
PASS   : TestQgsJsonUtils::testExportFeatureJson(Use old string concat)
RESULT : TestQgsJsonUtils::testExportFeatureJson():"Use old string concat":
     0.015 msecs per iteration (total: 64, iterations: 4096)
PASS   : TestQgsJsonUtils::testExportGeomToJson(Use json)
RESULT : TestQgsJsonUtils::testExportGeomToJson():"Use json":
     0.76 msecs per iteration (total: 98, iterations: 128)
PASS   : TestQgsJsonUtils::testExportGeomToJson(Use old string concat)
RESULT : TestQgsJsonUtils::testExportGeomToJson():"Use old string concat":
     0.85 msecs per iteration (total: 55, iterations: 64)
PASS   : TestQgsJsonUtils::cleanupTestCase()
@elpaso elpaso added Prototype For comment only, do not merge. Avoid using this tag and open a QEP instead! API API improvement only, no visible user interface changes Optimization I feel the need... the need for speed! labels Apr 18, 2019
@elpaso elpaso removed the Prototype For comment only, do not merge. Avoid using this tag and open a QEP instead! label May 6, 2019
@elpaso elpaso merged commit 9a612d4 into qgis:master May 10, 2019
@m-kuhn
Copy link
Member

m-kuhn commented May 14, 2019

Since this commit we're having troubles to compile QField.
Are the new headers (on which the geometry engine depends now if I'm not mistaken) installed alongside the qgis headers?

https://travis-ci.org/opengisch/QField/jobs/532227528#L1304

@elpaso
Copy link
Contributor Author

elpaso commented May 14, 2019

@m-kuhn no, the headers are only available in external and I don't think I installed anywhere. You might want the nlohmann/json_fwd.hpp (forward) or nlohmann/json.hpp.

@@ -3,6 +3,8 @@

SET(QGIS_CORE_SRCS
${CMAKE_SOURCE_DIR}/external/kdbush/include/kdbush.hpp
${CMAKE_SOURCE_DIR}/external/nlohmann/json.hpp
${CMAKE_SOURCE_DIR}/external/nlohmann/json_fwd.hpp
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these files need to be compiled? I think they are included anyway so no reason to put them into the SRCS here (and not sure about the kdbush either @nyalldawson )

Instead the external/CMakeFiles.txt should probably include the logic to install them onto the system since they are de facto no longer "external" for QGIS - apart from code organization inside the source tree.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although kdbush is a bit different, it's currently only used in a private header (which is not deployed on an installation), so that does not need to be shipped.

@m-kuhn
Copy link
Member

m-kuhn commented May 14, 2019

See followup, it's not that I might want something, it's that currently building standalone apps based on QGIS -devel packages is broken :)

@elpaso
Copy link
Contributor Author

elpaso commented May 14, 2019

@m-kuhn I made a quick test here and I think you can safely remove them from QGIS_CORE_SRCS

@m-kuhn
Copy link
Member

m-kuhn commented May 14, 2019

@elpaso the bigger issue is that they need to be installed.

@m-kuhn
Copy link
Member

m-kuhn commented May 14, 2019

@elpaso is the problem clear to you? What would you propose?

@elpaso
Copy link
Contributor Author

elpaso commented May 15, 2019

@elpaso is the problem clear to you?

@m-kuhn I'm not sure, because I've never built a C++ standalone application based on QGIS and I have no test cases.

If I understand this correctly, the problem is that in order to use QGIS libraries you need to include json_fwd.hpp and the externals headers are not installed anywhere the compiler can find them.

What would you propose?

Your suggestion (if I got that right) to install/copy the json headers to the QGIS include directory looks correct to me but as I said I cannot reproduce or test this issue.

@m-kuhn
Copy link
Member

m-kuhn commented May 15, 2019

Basically

are not installed anywhere the compiler can find them.

should be "are not installed anywhere".

Your suggestion (if I got that right) to install/copy the json headers to the QGIS include directory looks correct to me but as I said I cannot reproduce or test this issue.

It should be straightforward: when do you a ninja (or make) install a folder include/qgis is created in the install prefix. Inside this folder header files are installed. Currently qgsabstractgeometry.h looks like this #include "external/json_fwd.hpp", so the files need to be available inside include/qgis/external/ to be available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API improvement only, no visible user interface changes Optimization I feel the need... the need for speed!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants