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

Is there a way to forward declare nlohmann::json? #314

Closed
rachelnertia opened this issue Sep 21, 2016 · 41 comments
Closed

Is there a way to forward declare nlohmann::json? #314

rachelnertia opened this issue Sep 21, 2016 · 41 comments
Assignees
Labels
kind: question solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@rachelnertia
Copy link

It's preferable not to have to include large header files in other header files, but in my current project I've ended up with json.hpp included in a lot of headers because I can't find a way to forward declare the nlohmann::json type. It's made complicated by all the templating going on. I was wondering if there is actually a way that I just haven't found yet and, if there is, if it could maybe be included as part of the documentation/readme somewhere?

@gregmarr
Copy link
Contributor

Looks like this isn't possible for a user to do due to the default template parameters. The library would need to be updated to provide a forward declaration header with the default template parameters, and remove the defaults from the definition.

@SethHamilton
Copy link

You could try adding a #pragma once to the json.hpp right above #ifndef NLOHMANN_JSON_HPP. That pragma is supposed to ensure the file isn't even opened on subsequent inclusions. My experience is that compilers are pretty smart about the #ifndef.

@agauniyal
Copy link

Unless you're building your whole project each time, it shouldn't take much time. You could also use ccache to help speed up your build process.

@nlohmann
Copy link
Owner

nlohmann commented Oct 6, 2016

I think it would be quite cumbersome to realize a forward declaration. All I can think of right now would be a new header with a forward declaration defining a preprocessor symbol and adjusting the current json.hpp to use that preprocessor symbol to decide whether the default template parameters should be defined.

@nlohmann nlohmann added the state: help needed the issue needs help to proceed label Oct 10, 2016
@ralphtandetzky
Copy link

I added a file src/json_fwd.hpp in my forked repo. This header file is minimal and provides a forward declaration of nlohmann::basic_json and adds a typedef nlohmann::json. I've tested it with MinGW and it works. I've also made a pull request to the main repo, so I hope the new header file will be visible to the public there soon.

@nlohmann
Copy link
Owner

@ralphtandetzky Thanks! Just out of curiosity: Does the forward declaration header improve your compilation times? Or maybe @rachelnertia could also have a look at this?

@nlohmann nlohmann added state: please discuss please discuss the issue or vote for your favorite option improvement and removed state: help needed the issue needs help to proceed labels Oct 12, 2016
@jaredgrubb
Copy link
Contributor

Would it be possible to do something along these lines?
struct json : basic_json<...> { using basic_json::basic_json; };

I know this is not what std::string does, but it would solve the forward declare problem, and it would solve another annoyance I have. Whenever I try to use a debugger, I see a lot of ugly C++ symbols around json. It would be nice that you only have to "pay for" the symbol noise if you actually do something custom.

For example:

void std::__1::vector<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator>, std::__1::allocator<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator> > >::__push_back_slow_path<nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator> >(nlohmann::basic_json<std::__1::map, std::__1::vector, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> >, bool, long long, unsigned long long, double, std::__1::allocator>&&)

would become

void std::__1::vector<nlohmann::json, std::__1::allocator<nlohmann::json> >::__push_back_slow_path<nlohmann::json>(nlohmann::json&&)

@gregmarr
Copy link
Contributor

That wouldn't solve the forward declare problem, it would make it worse. You need to have the full definition of the base class to derive from it.

You can make your own struct that derives that way if you find that it helps with debugging, but I don't think changing the type of nlohmann::json at this point is a good idea.

@jaredgrubb
Copy link
Contributor

Yes, it would solve the forward declare problem because you have a distinct type:
namespace nlohmann { struct json; }
You can't do that now because you're not allowed to forward-declare a type alias (ie, typedef).

I agree there could be complications with changing the type, but it might be worth it. (It doesnt actually change convertibility or is-a relationships because 'json' still is-a 'basic_json' thing. But it is a ABI change by definition, which can have consequences).

@gregmarr
Copy link
Contributor

Ah, I misunderstood what you were suggesting.

@ralphtandetzky
Copy link

@nlohmann It did improve my compilation times on my project by a small margin. For my project where I use quite a bit of nlohmann::json for serialization purposes the compilation time improved from 2:44 to 2:40 in a one time test. I would guess that more than 50% of the cpp-files saw a declaration of nlohmann::json and about 15% included json.hpp directly or indirectly.
I was very careful in my project to reduce compile times to a minimum. I'm not using precompiled headers at the moment. I mostly include the C++ standard library, Qt and only little boost. I was very surprised that using json_fwd.hpp did not help more than that. But I would like to know, if others have more success with it. I have to remark that I observed some strange (counter-intuitive) effects on my compile-times lately. So my test might not be that relevant.

@rachelnertia
Copy link
Author

@nlohmann I don't have time at the moment to fully profile what difference it makes, but for the project which raised this question using json_fwd.hpp seems to have a negligible impact: in the single test I ran, I got a time of 29.46 seconds, down from 31.03, for a full build. So it doesn't appear to be a game changer.

@gregmarr
Copy link
Contributor

I'm not terribly surprised. The size of this library is tiny compared to the rest of the c++ standard library that you're probably already including.

@nlohmann
Copy link
Owner

I think the reported numbers do not justify merging PR #314.

@peku33
Copy link

peku33 commented Oct 23, 2016

I found this thread on google. In my solution where I use lots of .o objects with json. Compiling lots of independent files with forward declaration is noticeably faster than with full header. For now, fwd proposed by @ralphtandetzky works well. +1 for merging this

@nlohmann
Copy link
Owner

@peku33 Thanks for checking by! Do you have concrete numbers?

@peku33
Copy link

peku33 commented Oct 23, 2016

@nlohmann

test_fwd.cpp:

#include <json_fwd.hpp>

int main()
{
        return 0;
}

test_hpp.cpp:

#include <json.hpp>

int main()
{
        return 0;
}
time g++ -std=c++11 -O2 test_hpp.cpp
real    0m2.762s
user    0m2.585s
sys     0m0.175s
time g++ -std=c++11 -O2 test_fwd.cpp
real    0m0.649s
user    0m0.592s
sys     0m0.047s

For lightweight objects 2.762s vs 0.649s is the difference :)
For 100 it will be 4.5min vs 1min.

@peku33
Copy link

peku33 commented Nov 2, 2016

Any update?

@gregmarr
Copy link
Contributor

gregmarr commented Nov 2, 2016

@peku33 That's only the difference of that file included in isolation. The json.hpp file includes a lot of the STL. If you normally include a lot of the STL in your source files, then the difference should be a lot smaller.

#include <algorithm>
#include <array>
#include <cassert>
#include <cctype>
#include <ciso646>
#include <cmath>
#include <cstddef>
#include <cstdint>
#include <cstdlib>
#include <cstring>
#include <functional>
#include <initializer_list>
#include <iomanip>
#include <iostream>
#include <iterator>
#include <limits>
#include <locale>
#include <map>
#include <memory>
#include <numeric>
#include <sstream>
#include <stdexcept>
#include <string>
#include <type_traits>
#include <utility>
#include <vector>

It might be an interesting test to review if all of these includes are really necessary, especially all the c* headers.

@nlohmann
Copy link
Owner

nlohmann commented Nov 2, 2016

I recently used iwyu to check the headers and did not find headers that could be skipped. But maybe I overlooked something. I may add comments for at least one function of each header to document the actual need.

@peku33
Copy link

peku33 commented Nov 2, 2016

@gregmarr the problem is that I actually don't use heavy stl containers (string, vector, map, streams). That's why I'm posting here. As I mentioned before, the difference in compilation time is visible for me.

Because adding the fwd file wont change anything for users currently using the full hpp file - where is the problem with merging it?

@gregmarr
Copy link
Contributor

gregmarr commented Nov 2, 2016

@nlohmann sounds good. I hadn't heard of that site. I checked a couple of those manually, and they were all necessary. std::isdigit is used in one place, so we need cctype, for example.

@peku33 Just want to make sure that there would be some significant benefit for the extra complexity.

@nlohmann
Copy link
Owner

nlohmann commented Nov 2, 2016

@peku33 I'm still hesitant to add the header to the library. I shall have a look into it for the 3.0.0 release.

@nlohmann
Copy link
Owner

nlohmann commented Nov 2, 2016

FYI: I could actually remove <cstring>, see e385417.

@nlohmann
Copy link
Owner

nlohmann commented Nov 3, 2016

Update: <cstring> was actually used for strlen... :-S Sorry.

@marton78
Copy link

marton78 commented Dec 6, 2016

+1 for @jaredgrubb 's idea:

struct json : basic_json<>
{
    using basic_json<>::basic_json;
};

@gregmarr
Copy link
Contributor

gregmarr commented Dec 6, 2016

That was @jaredgrubb who posted that idea. This would be a breaking change, so can't be done before V3. However, you could do that yourself in a different namespace unless and until it's adopted here.

@jaredgrubb
Copy link
Contributor

I did try it locally, but it ended up not working well.

@marton78
Copy link

marton78 commented Dec 7, 2016

For me it works well, all I needed to do is add a constructor from basic_json<> to resolve an ambiguous conversion from nlohmann::json to my json when using json::parse:

namespace marton78
{
    struct json : nlohmann::basic_json<>
    {
        using basic_json<>::basic_json;
        json(basic_json<> const& j) : basic_json<>(j) {}
    };
}

@mcourteaux
Copy link

This would be a breaking change

@gregmarr Why would this be a breaking change? I assume this introduces a compatibility issue, but what exactly?

@peku33
Copy link

peku33 commented Jan 13, 2017

I didn't observe any drawbacks of using unmodified version of json (json.hpp) together with https://github.com/ralphtandetzky/json/blob/ralphtandetzky-json_fwd/src/json_fwd.hpp

@mcourteaux
Copy link

mcourteaux commented Jan 13, 2017

@peku33 I'm referring to the proposal of @jaredgrubb. The downside of the approach by @ralphtandetzky is that it still needs:

#include <cstdint>
#include <map>
#include <vector>
#include <string>

In terms of number of includes, the technique by @jaredgrubb seems way more attractive, but I'm wondering what the "breaking" issue is here.

@gregmarr
Copy link
Contributor

It changes the type of "nlohmann::json", so according to semver, it needs to wait for V3.0.0.

@peku33
Copy link

peku33 commented Jan 13, 2017

@mcourteaux Ok, sorry for my mistake. However, the heaviest (in terms of compilation time) part - streams (iostream, sstream) is removed.
Maybe it's off-topic, but using streams makes the executable really large on windows. File grows from 150kB to 600kB using mingw32.
@gregmarr How the type is changed by adding forward declaration?

@gregmarr
Copy link
Contributor

gregmarr commented Jan 13, 2017

I was responding to the message right above mine:

struct json : basic_json<>
{
    using basic_json<>::basic_json;
};

@stale
Copy link

stale bot commented Oct 25, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Oct 25, 2017
@stale stale bot closed this as completed Nov 1, 2017
@JoroRoss
Copy link

JoroRoss commented Nov 1, 2017

As an experiment, I disabled ccache, and built a 146 compilation-unit project I maintain with:

  1. #include<json.hpp> in (almost) every compilation unit - 3m17s
  2. #include<json_fwd.hpp> in (almost) every compilation unit - 2m4s
  3. include json_fwd.hpp/json.hpp only where needed - 1m40s

This was using the development branch of the nlohmann/json repo, where I extracted the forward declarations myself, along the lines of @ralphtandetzky's fork.

Even if the performance gain of using forward declarations was not quite this pronounced, the forward declarations should arguably be split out anyway: modular design promotes (re-) use, and should be encouraged as a software engineering best practice. Everything Lakos wrote about organizing code in large projects in https://www.amazon.com/Large-Scale-Software-Design-John-Lakos/dp/0201633620 still holds to this day.

Please reopen this ticket.

@peku33
Copy link

peku33 commented Nov 1, 2017

@JoroRoss what other headers did you include in your files?

I'm not 100% sure, but streams are pretty heavy to compile. If you use them anyway, there is probably going to be no significant gain.

In projects with lots of small compilation units, which does not include streams, this matters.

@JoroRoss
Copy link

JoroRoss commented Nov 3, 2017

Hey @peku33, thanks for getting back to me on this.

The code-base in question is reasonably well organized, limiting the use of large/complicated headers to compilation units that actually need them. I suppose <string> and <vector> end up in a lot of places, but we try to keep them in implementation files where possible. We (of course) use <iosfwd> where appropriate - most compilation units will pick that up some way or other, as we declare iostream operators with most of our data model classes. Only the data model compilation units (and a hand-full of collaborators) need the actual definitions.

For our own templates, we typically create two headers - one with declarations for clients to pick up, another with definitions. We often introduce dedicated compilation units with template instantiations for the production classes, and include the definition files in a unit tests compilation unit for mocking/stubbing. (We used to not do this; when we put in the effort, we got build times down from 25 minutes to around 3.)

So yeah, sort of a typical codebase, that encapsulates the use of expensive resources.

Regarding your speculations about the compilation time of <iostream>, I'm afraid to say you are wrong. That is, unless every single compilation unit in your project needs every last template definition from <streambuf>, <ostream>, <istream> and <fstream>... (if you're in that situation, you may want to consider restructuring your project!).

The overhead in build times typically comes from (redundant) parsing of headers, not from compilation (unused template declarations don't get compiled, just parsed). The nlohmann/json project luckily uses <iosfwd>, and including those forward declarations in all compilation units has an almost negligible impact on compilation times. Hardly any compilation units actually need to include the actual definitions, and even if they do, they can often limit the impact by specifically including <ostream>, <istream> or <fstream>.

For what it's worth: I just repeated my experiment from before, this time including <iostream> in (almost) every compilation unit. The build time went to 2m20s. My test is producing a CMake Debug build, so the build time is dominated by IO and parsing. The pre-compiler emits about twice as much code for the json header as it does for the iostream one (as can be readily verified by running g++ with the -E flag.)

@nlohmann nlohmann removed state: please discuss please discuss the issue or vote for your favorite option state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated labels Jan 28, 2018
@nlohmann
Copy link
Owner

As PR #700 is merged, forward declaration is now possible. It will be part of 3.1.0 to be released next week.

@nlohmann nlohmann added this to the Release 3.1.0 milestone Jan 28, 2018
@nlohmann nlohmann self-assigned this Jan 28, 2018
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Jan 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: question solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests