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

There is performance inefficiency found by coverity tool json2.1.1/include/nlohmann/json.hpp #673

Closed
qinglucich opened this issue Aug 1, 2017 · 8 comments
Assignees
Labels
kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@qinglucich
Copy link

4063 @SInCE version 1.0.0
4064 */
4065 template<class ValueType, typename std::enable_if<
4066 std::is_convertible<basic_json_t, ValueType>::value, int>::type = 0>

CID 10413 (#1 of 1): Big parameter passed by value (PASS_BY_VALUE)
pass_by_value: Passing parameter default_value of type cerebras::file_metadata (size 168 bytes) by value.
4067 ValueType value(const typename object_t::key_type& key, ValueType default_value) const
4068 {

@nlohmann
Copy link
Owner

nlohmann commented Aug 1, 2017

Thanks for reporting. Here is the line in context: https://github.com/nlohmann/json/blob/v2.1.1/src/json.hpp#L4066

@nlohmann
Copy link
Owner

nlohmann commented Aug 1, 2017

This is also relevant for 3.0.0.

@nlohmann nlohmann self-assigned this Aug 1, 2017
nlohmann added a commit that referenced this issue Aug 1, 2017
@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Aug 1, 2017
@nlohmann
Copy link
Owner

nlohmann commented Aug 1, 2017

Fixed. Thanks for reporting!

@nlohmann nlohmann closed this as completed Aug 1, 2017
@murderotica
Copy link

murderotica commented Aug 11, 2017

Wouldn't using universal reference instead of const& be better (in case of an r-value a move will be possible instead of always copy)?

This
ValueType value(const json_pointer& ptr, const ValueType& default_value)
replaced with this

ValueType value(const json_pointer& ptr, ValueType&& default_value) { //... JSON_CATCH (out_of_range&) { return std::forward<ValueType>(default_value); } }

reference example https://godbolt.org/g/Yq2euX

@theodelrieu
Copy link
Contributor

There is quite a lot of room for zero-copy/rvalue references support in the library indeed.

Note that the return type should be changed to auto (...) -> decltype(std::forward<ValueType>(default_value)) to avoid copies.

@nlohmann nlohmann reopened this Aug 11, 2017
@nlohmann
Copy link
Owner

@murderotica, @theodelrieu Can you create a PR for these optimizations?

@theodelrieu
Copy link
Contributor

I'll add that to my TODO list.

@nlohmann
Copy link
Owner

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

4 participants