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

The header file is big, can we use what we need. #2134

Closed
dota17 opened this issue May 25, 2020 · 8 comments
Closed

The header file is big, can we use what we need. #2134

dota17 opened this issue May 25, 2020 · 8 comments
Labels
kind: enhancement/improvement state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated

Comments

@dota17
Copy link
Contributor

dota17 commented May 25, 2020

Which feature do you want to see in the library?

The header file is big, and it has many extended features, such as binary formats (BSON, CBOR, MessagePack, and UBJSON), json patch.

However, sometimes we don't need the extended features.
Is there a way that we do not include these extended features so that the header file is small and when the extended features have bugs, we don't need to fix it.

How would the feature be usable for other users?

In some cases, the development guidelines we followed says that if the feature code has bugs, you must fix it whatever you use or not. Beacuse it provides a possible way to attack the program.

If we can use what we need or not include extended code , we can avoid attacking and don't need to spend times to fix it. And we can only focus on the changes in the core modules which we care about.

@nlohmann
Copy link
Owner

This would be possible, but then we would juggle with some preprocessor macros to exclude undesired features.

@FrancoisChabot
Copy link
Contributor

FrancoisChabot commented May 26, 2020

The underlying rationale of this request is confusing to me.

If the extra code could be removed without affecting the use of the library within a given program, then that code is not part of the program in the first place and does not participate in the program's attack surface.

C++ has a fundamental "only pay for what you use" principle at play here. including code does not mean that it participates in the program, you have to actually USE the code for it to actually have an effect.

@nlohmann
Copy link
Owner

What I can think about in the long run is to better separate the internals of the JSON class from free functions working on JSON values. For instance, the binary formats are currently tied to the JSON class as from_cbor etc. are static functions. If we could move this into separate headers, then users could decide to include a binary.hpp header when they need to read or write a binary format, but can work without it otherwise. But this would need some measurement (Is it really worthwhile?) and cannot be done without breaking the API. Hence I could only think about preprocessor magic.

But you are right - the argument of excluding buggy code is not really motivating that effort.

@deepbluev7
Copy link

One reason why I personally would love to have reduced headers is compile times. nlohmann/json is pretty much the highest compile time cost in our code base (next to std::variant), so if I could only include, what I actually need, that would probably at least half our compile time and memory usage. Some data for that from a basic ClangBuildAnalyzer run:

**** Templates that took longest to instantiate:
109129 ms: mtx::events::to_json<mtx::events::msg::RoomKey> (100 times, avg 1091 ms)
 79308 ms: nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_stri... (52 times, avg 1525 ms)
 72257 ms: nlohmann::detail::parser<nlohmann::basic_json<std::map, std::vector,... (52 times, avg 1389 ms)
 48398 ms: nlohmann::detail::parser<nlohmann::basic_json<std::map, std::vector,... (52 times, avg 930 ms)
 31169 ms: std::visit<(lambda at ../src/Utils.h:198:27), const std::variant<mtx... (32 times, avg 974 ms)
 31098 ms: std::__do_visit<false, true, (lambda at ../src/Utils.h:198:27), cons... (32 times, avg 971 ms)
 29901 ms: std::__detail::__variant::__gen_vtable_impl<true, std::__detail::__v... (32 times, avg 934 ms)
 29841 ms: std::__detail::__variant::__gen_vtable_impl<true, std::__detail::__v... (32 times, avg 932 ms)
 28765 ms: nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_stri... (214 times, avg 134 ms)
 27706 ms: nlohmann::basic_json<std::map, std::vector, std::__cxx11::basic_stri... (214 times, avg 129 ms)
 26373 ms: std::visit<(lambda at ../src/Utils.h:192:27), const std::variant<mtx... (32 times, avg 824 ms)
 25925 ms: std::__do_visit<false, true, (lambda at ../src/Utils.h:192:27), cons... (32 times, avg 810 ms)
 25352 ms: std::__detail::__variant::__gen_vtable_impl<true, std::__detail::__v... (32 times, avg 792 ms)
 25309 ms: std::__detail::__variant::__gen_vtable_impl<true, std::__detail::__v... (32 times, avg 790 ms)
 23926 ms: spdlog::logger::log<> (26 times, avg 920 ms)
 23770 ms: std::unique_ptr<std::__cxx11::basic_string<char>, (lambda at /usr/in... (921 times, avg 25 ms)
 23712 ms: fmt::v6::format_to<fmt::v6::basic_string_view<char>, 250, char> (26 times, avg 912 ms)
 23528 ms: fmt::v6::internal::vformat_to<char> (26 times, avg 904 ms)
 23232 ms: fmt::v6::vformat_to<fmt::v6::arg_formatter<fmt::v6::buffer_range<cha... (26 times, avg 893 ms)
 21931 ms: nlohmann::detail::json_sax_dom_callback_parser<nlohmann::basic_json<... (52 times, avg 421 ms)
 21524 ms: std::__uniq_ptr_impl<std::__cxx11::basic_string<char>, (lambda at /u... (921 times, avg 23 ms)
 19514 ms: fmt::v6::format_handler<fmt::v6::arg_formatter<fmt::v6::buffer_range... (26 times, avg 750 ms)
 18838 ms: std::is_nothrow_move_constructible<nlohmann::basic_json<std::map, st... (52 times, avg 362 ms)
 18763 ms: std::__is_nothrow_move_constructible_impl<nlohmann::basic_json<std::... (52 times, avg 360 ms)
 18761 ms: std::is_nothrow_constructible<nlohmann::basic_json<std::map, std::ve... (52 times, avg 360 ms)
 18751 ms: std::__and_<std::is_constructible<nlohmann::basic_json<std::map, std... (52 times, avg 360 ms)
 18571 ms: std::is_constructible<nlohmann::basic_json<std::map, std::vector, st... (52 times, avg 357 ms)
 17649 ms: std::variant<mtx::events::StateEvent<mtx::events::state::Aliases>, m... (50 times, avg 352 ms)
 15400 ms: std::__detail::__variant::_Variant_base<mtx::events::StateEvent<mtx:... (50 times, avg 308 ms)
 14913 ms: std::is_constructible<nlohmann::basic_json<std::map, std::vector, st... (52 times, avg 286 ms)

**** Template sets that took longest to instantiate:
225586 ms: mtx::events::to_json<$> (1398 times, avg 161 ms)
177339 ms: nlohmann::basic_json<$>::basic_json<$> (2642 times, avg 67 ms)
175520 ms: nlohmann::adl_serializer<$>::to_json<$> (2642 times, avg 66 ms)
148215 ms: std::__and_<$> (47282 times, avg 3 ms)
117900 ms: std::__do_visit<$> (243 times, avg 485 ms)
113719 ms: std::__detail::__variant::__gen_vtable_impl<$>::_S_apply (243 times, avg 467 ms)
113251 ms: std::__detail::__variant::__gen_vtable_impl<$>::_S_apply_all_alts<$> (243 times, avg 466 ms)
110391 ms: std::__detail::__variant::__gen_vtable_impl<$>::_S_apply_single_alt<$> (5832 times, avg 18 ms)
104455 ms: std::visit<$> (202 times, avg 517 ms)
 94155 ms: std::__detail::__variant::__gen_vtable_impl<$>::__visit_invoke (5873 times, avg 16 ms)
 89492 ms: std::__detail::__variant::__gen_vtable_impl<$>::__do_visit_invoke (5873 times, avg 15 ms)
 85145 ms: std::__detail::__variant::__gen_vtable_impl<$>::__visit_invoke_impl (5873 times, avg 14 ms)
 84379 ms: nlohmann::basic_json<$>::create<$> (1174 times, avg 71 ms)
 79308 ms: nlohmann::basic_json<$>::parse<$> (52 times, avg 1525 ms)
 72257 ms: nlohmann::detail::parser<$>::parse (52 times, avg 1389 ms)
 62895 ms: std::__or_<$> (39517 times, avg 1 ms)
 59762 ms: nlohmann::detail::to_json<$> (1119 times, avg 53 ms)
 59056 ms: nlohmann::detail::external_constructor<$>::construct<$> (1119 times, avg 52 ms)
 57211 ms: std::is_trivially_destructible<$> (12865 times, avg 4 ms)
 55331 ms: nlohmann::detail::parser<$>::sax_parse_internal<$> (104 times, avg 532 ms)
 53454 ms: std::is_destructible<$> (13177 times, avg 4 ms)
 49860 ms: qIsRelocatable<$> (10618 times, avg 4 ms)
 46011 ms: std::is_constructible<$> (7222 times, avg 6 ms)
 39074 ms: std::unique_ptr<$> (1425 times, avg 27 ms)
 35613 ms: std::__invoke_result<$> (8196 times, avg 4 ms)
 34879 ms: std::__uniq_ptr_impl<$> (1425 times, avg 24 ms)
 31188 ms: std::__result_of_impl<$> (6773 times, avg 4 ms)
 29743 ms: std::is_convertible<$> (19808 times, avg 1 ms)
 28283 ms: nlohmann::basic_json<$>::basic_json (209 times, avg 135 ms)
 27706 ms: nlohmann::basic_json<$>::json_value::json_value (214 times, avg 129 ms)

The mtx::events::to_json functions are super simple and just converts a struct with around 3 members to json, but they are called with around 20 different template arguments. I see no easy way to optimize it apart from simplifying the nlohmann::json template/type itself. If it takes half as long to compile, we could probably reduce our compile time by 25%, which would be pretty substantial and make development much more enjoyable.

@stale
Copy link

stale bot commented Jul 3, 2020

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 Jul 3, 2020
@stale stale bot closed this as completed Jul 11, 2020
@deepbluev7
Copy link

Well, I think this issue still applies, but I guess nothing will be done to change that? (which is fine, there are probably better things to focus on.)

@nlohmann
Copy link
Owner

No, I'm afraid this is as good as it gets. A shallow clone is cheap now, and anything beyond that would mean changing the repository's history.

@deepbluev7
Copy link

Well it still applies that including and using nlohmann/json.hpp has a large compile time overhead, which is unrelated to the download times, but I lack the expertise to say, if that is something that can be easily fixed without changing what nlohmann/json is about and what makes it great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated
Projects
None yet
Development

No branches or pull requests

4 participants