Optimize copy / assignment and size of json iterator by sjanel · Pull Request #3452 · nlohmann/json

sjanel · 2022-04-23T21:49:06Z

Hello,

I stambled accross this clang-tidy warning in my project and investigated further, it being a json::const_iterator:

the parameter 'it' is copied for each invocation but only used as a const reference; consider making it a const reference [performance-unnecessary-value-param]

It looks like json::iterator is actually non trivially copyable (and quite big), which is unexpected for an iterator and the reason why clang-tidy is complaining. In templated code it is common practice to pass iterators by value and not by reference. After investigation and reading of the code I realized that it was made like that at the beginning as a workaround for a MSVC bug (see #1608). It is sad to impact all other compilation modes and compilers just for a MSVC bug.

I redesigned internal_iterator_t with two distinct implementations, one optimized in the case primitive_iterator_t, object_t::iterator and array_t::iterator are all trivially copyable (standard case, mainly for non-MSVC compilers), one for the general case (mainly used by MSVC, and other compilers with exotic vector and/or map iterator implementations).

This is the fix that I propose, in the hope that since then, the MSVC issue is not a concern anymore. After this PR, all operations on iterators are expected to be faster and iterators can be passed by value without any performance impact like standard operators.

nlohmann · 2022-04-23T21:59:12Z

Thanks! It seems @falbrechtskirchinger is working in #3446 on similar issues - can you please check whether his adjustments go in the same direction?

coveralls · 2022-04-23T23:08:03Z

Coverage remained the same at 100.0% when pulling 49badfc on sjanel:feature/defaultcopyconstructibleiterator into b21c345 on nlohmann:develop.

falbrechtskirchinger · 2022-04-24T07:13:02Z

Remove default value initialization, which does not seem to be used, such that union is valid

Made iter_impl copy construction default, such that it is trivially copyable (did the same for copy assignment)

Some of this is probably redundant due to my pending PR. Will look at the code in a second.
Edit: Actually I hardly touched iter_impl my fixes target proxy iteration.

This is the fix that I propose, in the hope that since then, the MSVC issue is not a concern anymore. After this PR, all operations on iterators are expected to be faster and iterators can be passed by value without any performance impact like standard operators.

MSVC still can't handle it:
"error C2338: union cannot be safely used with non trivial types"

But I agree that pessimizing this for everyone else is bad practice.

PS: I don't know why, but formatter slightly changes output of the file here and there... is it coming from my clang-format version (I use v14) ? Please help me setting my environment with the expected formatter settings

This project uses Artistic Style to format code which is executed by make amalgamate or make pretty. You should disable clang-format when working on this code.

sjanel · 2022-04-24T14:08:27Z

Thanks! It seems @falbrechtskirchinger is working in #3446 on similar issues - can you please check whether his adjustments go in the same direction?

Thanks for your comment. I checked the PR and it's not exactly the same thing, it's more about preparing for C++20 support with ranges. This PR is mainly an optimization of the json::iterator type.

sjanel · 2022-04-24T14:10:11Z

Remove default value initialization, which does not seem to be used, such that union is valid

Made iter_impl copy construction default, such that it is trivially copyable (did the same for copy assignment)

Some of this is probably redundant due to my pending PR. Will look at the code in a second. Edit: Actually I hardly touched iter_impl my fixes target proxy iteration.

This is the fix that I propose, in the hope that since then, the MSVC issue is not a concern anymore. After this PR, all operations on iterators are expected to be faster and iterators can be passed by value without any performance impact like standard operators.

MSVC still can't handle it: "error C2338: union cannot be safely used with non trivial types"

But I agree that pessimizing this for everyone else is bad practice.

PS: I don't know why, but formatter slightly changes output of the file here and there... is it coming from my clang-format version (I use v14) ? Please help me setting my environment with the expected formatter settings

This project uses Artistic Style to format code which is executed by make amalgamate or make pretty. You should disable clang-format when working on this code.

Thanks for taking a look at my PR! I am currently working on a cleaner iter_impl redesign to handle general case and (hopefully) fix MSVC compilation error. It complains because it does not like union with non trivial types (even if they are trivially copyable).

falbrechtskirchinger · 2022-04-24T19:15:51Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+               : std::alignment_of<ObjectIterator>::value);
+
+  protected:
+    typename std::aligned_storage<kMaxIteratorSize, kMaxIteratorAlignment>::type m_data;


Please use alignas(kMaxIteratorAlignment) std::uint8_t m_data[kMaxIteratorSize];

std::aligned_storage has been deprecated.

I am working on this PR on my fork. Besides Windows weird issues, this change triggers some warnings (as error):

/__w/json/json/include/nlohmann/detail/iterators/internal_iterator.hpp:67:48: error: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays,-warnings-as-errors]

Looks like a bit too much to use std::array here IMO.

You can annotate the line with
// NOLINT(cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays) to suppress the error.

NOLINTNEXTLINE works too, if you place it above the statement and not on the same line (probably better here).

PR is ready for review (Microsoft issues fixed ;)), although I still have different formatter output with make amalgamate (my astyle version is Artistic Style Version 3.1). Is it an issue, and if so, why do I have all these differences ? (see include/nlohmann/json.hpp for instance)

falbrechtskirchinger

Some initial thoughts.
The formatting of json.hpp is very distracting. I scrolled through the file very quickly and probably missed most of your changes.

falbrechtskirchinger · 2022-04-27T11:36:48Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+    internal_iterator_impl(const internal_iterator_impl& o) noexcept
+        : m_it_type(o.m_it_type)
+    {
+        switch (m_it_type)


Any particular reason you're not using an enum class[ : std::uint8_t] here?

falbrechtskirchinger · 2022-04-27T11:41:26Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+                ::new (&this->primitive_iterator()) primitive_iterator_t(o.primitive_iterator());
+                break;
+            default:
+                break;


This should at least be:

default: // LCOV_EXCL_LINE JSON_ASSERT(false); // LCOV_EXCL_LINE

Possibly even:

default: // LCOV_EXCL_LINE JSON_ASSERT(false); // NOLINT(cert-dcl03-c,hicpp-static-assert,misc-static-assert) LCOV_EXCL_LINE

falbrechtskirchinger · 2022-04-27T11:41:38Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+                ::new (&this->primitive_iterator()) primitive_iterator_t(std::move(o.primitive_iterator()));
+                break;
+            default:
+                break;


falbrechtskirchinger · 2022-04-27T11:41:49Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+                    this->primitive_iterator() = o.primitive_iterator();
+                    break;
+                default:
+                    break;


And here. You get the idea...

falbrechtskirchinger · 2022-04-27T11:54:54Z

include/nlohmann/detail/macro_scope.hpp

+    #if !defined(__clang__) && defined(__GNUC__) && (__GNUC__ * 10000) < 50000
+        #undef JSON_HAS_TRIVIALLY_COPYABLE
+    #endif
+#endif


a) For it to be overridable, you should define it to 0 or 1, just like the other JSON_HAS_* macros.
b) Use the Hedley version check macro: JSON_HEDLEY_GNUC_VERSION_CHECK(5, 0, 0).
c) (For later:) Remember to document it in docs/mkdocs/docs/api/macros/index.md + docs/mkdocs/docs/api/macros/json_has_trivially_copyable.md.

falbrechtskirchinger · 2022-04-27T12:01:28Z

include/nlohmann/json.hpp

+#include <numeric>     // accumulate
+#include <string>      // string, stoi, to_string
+#include <utility>     // declval, forward, move, pair, swap
+#include <vector>      // vector


About the formatting: I assume you reformatted with clang-format and then used astyle?
astyle does not produce consistent output for equal source input. It keeps some of the input formatting, especially some white space as you can see here.

I think you'll have to find a way to undo that formatting. Otherwise reviewing and resolving merge conflicts isn't going to be much fun.

"astyle does not produce consistent output for equal source input." Now I understood the issue, yes I have clang-format on save by default in my IDE. I did not know it would harm astyle format afterwards. I will fix all the formatting issues manually, sorry for the inconvenience.

falbrechtskirchinger · 2022-04-27T12:07:52Z

include/nlohmann/json.hpp


        // This could have been written as:
-        // result.m_it.array_iterator = m_value.array->insert(pos.m_it.array_iterator, cnt, val);
+        // result.m_it.set_array_iterator(m_value.array->insert(pos.m_it.array_iterator(), cnt, val));


I've come across a related Codacy issue today ... both things could be solved with a helper. Doesn't have to be here though.

falbrechtskirchinger · 2022-04-27T12:10:04Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+                                                        : sizeof(object_iterator_t))];
+#endif
+    } m_data {};
+};


Horrible, indeed. I think I have a better solution. Will get back to you when I have more time.

This should work:

https://godbolt.org/z/MGhhEsd3G

namespace detail { template<std::size_t... Is> struct max; template<> struct max<> { using type = std::integral_constant<std::size_t, 0>; }; template<std::size_t I, std::size_t... Rest> struct max<I, Rest...> { using rest_type = typename max<Rest...>::type; using type = typename std::conditional< (I > rest_type::value), std::integral_constant<std::size_t, I>, rest_type>::type; }; template<typename... Ts> using max_alignof = typename max<alignof(Ts)...>::type; template<typename... Ts> using max_sizeof = typename max<sizeof(Ts)...>::type; } constexpr std::size_t data_align = detail::max_alignof<int, double, char>::value; constexpr std::size_t data_size = detail::max_sizeof<int, double, char>::value; alignas(data_align) std::uint8_t data[data_size];

~~Maybe add a new file meta/algorithm.hpp or similar.~~
Edit: On second thought, type_traits.hpp should be fine.

nlohmann · 2022-05-01T07:43:23Z

Please update to the latest develop branch (we renamed some folders in #3462).

sjanel · 2022-05-01T07:55:09Z

Please update to the latest develop branch (we renamed some folders in #3462).

Ok, done

nlohmann · 2022-05-01T08:01:21Z

include/nlohmann/detail/iterators/internal_iterator.hpp

 namespace detail
 {
+
+template<class ObjectIterator, class ArrayIterator>


Please add some documentation on the idea behind the class.

nlohmann · 2022-05-01T08:02:02Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+    {
+        internal_data_t() noexcept = default;
+
+#ifdef JSON_HAS_CPP_14


Please document the idea behind this piece of code.

And a note about the std::aligned_storage deprecation in C++23.

falbrechtskirchinger

I'd still prefer something like my proposed max_alignof and max_sizeof traits over nested tertiary operators and separate code paths for C++11 and C++14 and up.

falbrechtskirchinger · 2022-05-01T08:07:44Z

include/nlohmann/detail/iterators/internal_iterator.hpp

+*/
+template<class ObjectIterator, class ArrayIterator>
+using internal_iterator = internal_iterator_impl < ObjectIterator, ArrayIterator,
+#if defined(__clang__) || !defined(JSON_HEDLEY_GNUC_VERSION) || JSON_HEDLEY_GNUC_VERSION_CHECK(5, 0, 0)


This condition isn't needed anymore, right? It's handled in your type trait implementation.

falbrechtskirchinger · 2022-05-01T08:10:58Z

include/nlohmann/detail/meta/type_traits.hpp

    return value;
 }

+#if defined(__clang__) || !defined(JSON_HEDLEY_GNUC_VERSION) || JSON_HEDLEY_GNUC_VERSION_CHECK(5, 0, 0)


This can be simplified to:

#if !defined(JSON_HEDLEY_GNUC_VERSION) || JSON_HEDLEY_GNUC_VERSION_CHECK(5, 0, 0)

nlohmann

Please address @falbrechtskirchinger 's comments.

sjanel requested a review from nlohmann as a code owner April 23, 2022 21:49

sjanel force-pushed the feature/defaultcopyconstructibleiterator branch 2 times, most recently from 20ce876 to a9b7161 Compare April 24, 2022 18:57

falbrechtskirchinger reviewed Apr 24, 2022

View reviewed changes

sjanel force-pushed the feature/defaultcopyconstructibleiterator branch from a9b7161 to 9878d78 Compare April 24, 2022 20:58

sjanel marked this pull request as draft April 25, 2022 19:38

sjanel marked this pull request as ready for review April 27, 2022 11:18

sjanel force-pushed the feature/defaultcopyconstructibleiterator branch from 9878d78 to 32100a0 Compare April 27, 2022 11:19

falbrechtskirchinger suggested changes Apr 27, 2022

View reviewed changes

sjanel force-pushed the feature/defaultcopyconstructibleiterator branch 2 times, most recently from fa038d1 to 09849a6 Compare April 30, 2022 11:58

nlohmann added the please rebase Please rebase your branch to origin/develop label May 1, 2022

sjanel force-pushed the feature/defaultcopyconstructibleiterator branch from 09849a6 to 49badfc Compare May 1, 2022 07:54

nlohmann removed the please rebase Please rebase your branch to origin/develop label May 1, 2022

nlohmann reviewed May 1, 2022

View reviewed changes

falbrechtskirchinger suggested changes May 1, 2022

View reviewed changes

nlohmann requested changes May 1, 2022

View reviewed changes

sjanel closed this Nov 26, 2022

sjanel force-pushed the feature/defaultcopyconstructibleiterator branch from 49badfc to a3e6e26 Compare November 26, 2022 09:22

Uh oh!

Conversation

sjanel commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nlohmann commented Apr 23, 2022

Uh oh!

coveralls commented Apr 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

falbrechtskirchinger commented Apr 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjanel commented Apr 24, 2022

Uh oh!

sjanel commented Apr 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjanel Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

falbrechtskirchinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjanel Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

falbrechtskirchinger Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlohmann commented May 1, 2022

Uh oh!

sjanel commented May 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

falbrechtskirchinger left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nlohmann left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

sjanel commented Apr 23, 2022 •

edited

Loading

coveralls commented Apr 23, 2022 •

edited

Loading

falbrechtskirchinger commented Apr 24, 2022 •

edited

Loading

sjanel Apr 25, 2022 •

edited

Loading

sjanel Apr 27, 2022 •

edited

Loading

falbrechtskirchinger Apr 27, 2022 •

edited

Loading