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

Added forward declaration header json_forward.hpp. Minimise build times. #823

Closed
wants to merge 1 commit into from
Closed

Added forward declaration header json_forward.hpp. Minimise build times. #823

wants to merge 1 commit into from

Conversation

zerodefect
Copy link
Contributor

This pull request aims to reduce compile time by the introduction of a header dedicated to providing forward declaration of nlohmann::json.

One of the great merits of the this library is that it is header only. Unfortunately, that does also come with some disadvantages, namely the compile times are often higher with such code dependencies.

This pull request introduces a 'json_forward.hpp' header file (similar to how Boost offer forward header files) in an attempt to try reduce build times.

Things to note (and some of these come down to personal preference):

  • json.hpp includes json_forward.hpp to minimise code replication
  • json.hpp includes some of same std library headers as json_forward.hpp (technically these could be removed) but with #include guards, I don't feel this is much of an issue. Discuss.

Points to consider:

  • Code change is small.
  • It reduces build times (at least what I've seen).

So I tried integrating these code changes with my own project and ran a comparison test.

Build time without forward declaration header:

real 10m7.981s
user 36m40.768s
sys 2m1.432s

Build time making extensive use of forward declaration header (where possible):

real 9m21.282s
user 33m53.744s
sys 1m52.880s

Please note that this test was not wholly scientific. I imagine mileage will vary across environments/compilers and the level of use of this library in the said project.

Tested with cmake and gcc 6.3 running on Ubuntu's 17.04 (Zesty).

@theodelrieu
Copy link
Contributor

Hi! This is already done in #700, although not merged (yet).

@zerodefect
Copy link
Contributor Author

Thanks @theodelrieu, I see the changes in your pull request are numerous. Impressive! Any idea when your changes may be merged?

Could my changes function as a half-way house? Do your changes preclude my changes from being merged? Further clarification would be much appreciated.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 08d5d76 on zerodefect:add_forward_declaration_hdr into 4c4f60f on nlohmann:develop.

@theodelrieu
Copy link
Contributor

@zerodefect Soon I hope! Well the main issue is that we still want to provide an amalgamated version of json.hpp (you can see it in my PR) for people that still wish to use the single header. With your changes, users are forced to download two files.

All of that work is already done on #700, I need to rebase to solve minor conflicts.

Meanwhile you can use the json_fwd.hpp file on your projects, that's what we do at my work while waiting for the 3.0 release.

If you have more questions, let me know :)

Repository owner deleted a comment from trilogy-service-ro Dec 6, 2017
@@ -29,6 +29,10 @@ SOFTWARE.
#ifndef NLOHMANN_JSON_HPP
#define NLOHMANN_JSON_HPP


#include "json_forward.hpp" // Some forward declarations are re-used here.
Copy link
Owner

Choose a reason for hiding this comment

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

I do not like breaking the header into two files at this moment. PR #700 proposes a restructuring into several files, but also provides a script to re-create a single header.

@nlohmann
Copy link
Owner

nlohmann commented Dec 6, 2017

Sorry for not commenting earlier. As I wrote in #823 (review), I do not like the idea of splitting the header into several files. Furthermore, PR #700 already goes further and not only splits the code, but also provides a script to re-create a single header. Unless there I missed a strong argument for this PR, I would close it.

@nlohmann nlohmann added the state: please discuss please discuss the issue or vote for your favorite option label Dec 6, 2017
@zerodefect zerodefect closed this Dec 14, 2017
@zerodefect zerodefect deleted the add_forward_declaration_hdr branch December 14, 2017 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state: please discuss please discuss the issue or vote for your favorite option
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants