-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
Comments
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. |
You could try adding a |
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. |
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 |
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. |
@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? |
Would it be possible to do something along these lines? 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:
would become
|
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 |
Yes, it would solve the forward declare problem because you have a distinct type: 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). |
Ah, I misunderstood what you were suggesting. |
@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. |
@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. |
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. |
I think the reported numbers do not justify merging PR #314. |
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 |
@peku33 Thanks for checking by! Do you have concrete numbers? |
test_fwd.cpp:
test_hpp.cpp:
For lightweight objects 2.762s vs 0.649s is the difference :) |
Any update? |
@peku33 That's only the difference of that file included in isolation. The
It might be an interesting test to review if all of these includes are really necessary, especially all the c* headers. |
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. |
@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? |
@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. |
FYI: I could actually remove |
Update: |
+1 for @jaredgrubb 's idea: struct json : basic_json<>
{
using basic_json<>::basic_json;
}; |
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. |
I did try it locally, but it ended up not working well. |
For me it works well, all I needed to do is add a constructor from namespace marton78
{
struct json : nlohmann::basic_json<>
{
using basic_json<>::basic_json;
json(basic_json<> const& j) : basic_json<>(j) {}
};
} |
@gregmarr Why would this be a breaking change? I assume this introduces a compatibility issue, but what exactly? |
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 |
@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. |
It changes the type of "nlohmann::json", so according to semver, it needs to wait for V3.0.0. |
@mcourteaux Ok, sorry for my mistake. However, the heaviest (in terms of compilation time) part - streams ( |
I was responding to the message right above mine:
|
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. |
As an experiment, I disabled ccache, and built a 146 compilation-unit project I maintain with:
This was using the development branch of the 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. |
@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. |
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.) |
As PR #700 is merged, forward declaration is now possible. It will be part of 3.1.0 to be released next week. |
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?
The text was updated successfully, but these errors were encountered: