replaced static const/fixed std::vector containers with std::array

[firewave]

Scanning mame_regtest with --enable=all --inconclusive and DISABLE_VALUEFLOW=1:`

Clang 14 1,901,904,224 -> 1,899,570,897 -> 1,899,550,337

[firewave]

I filed https://trac.cppcheck.net/ticket/11292 about detecting this.

[chrchr-github]

Do you have any intuition why this would be faster?

[firewave]

Do you have any intuition why this would be faster?

I looked at the profiling output and it uses less instructions but there's no indication where the performance is won.

I posted the llvm-mca output in the ticket and that also shows a win.

<rambling>
Looking at the bytecode in Godbolt it does loop unrolling for both cases but there's less stuff done for the std::array. I assume since there's no actual container is involved there's better folding/inling. I am so bad at understanding assembler that I can't tell what is actually different.

Looking at the IR gives a bit more insight. It is not doing any std::vector stuff, seems like it lowers the std::string a bit, needs no exception handling and doesn't need to track lifetime. So it seems like even if the std::vector is constant it cannot be completely eliminated thus using std:array eliminates that overhead.
</rambling>

So my "intuition" (actually something I consider obvious) is using a lighter (non-)container makes things lighter overall.

[chrchr-github]

Thanks. I would not have thought that there is anything to gain here after optimization.

[firewave]

Thanks. I would not have thought that there is anything to gain here after optimization.

The compilers are able to inline things from STL containers to a varying degree. I also added data for GCC 12 to the ticket.

[pfultz2]

It would be better if we had a make_array function so we could write it as static const auto x = make_array<T>(...), so we dont have to update the size when we add a new element.

[firewave]

It would be better if we had a make_array function so we could write it as static const auto x = make_array<T>(...), so we dont have to update the size when we add a new element.

But we have a std::make_pair which makes the code slower than doing it via a constructor - gotta love that modern C++.

Will give it a spin, but I guess for those 4 occurrences it seems unnecessary. In isPrefixStringCharLiteral() we could also inline the variable.

Too small arrays are reported by the compiler, too big ones aren't (I thought a filed a ticket about detecting that already).

[firewave]

It would be better if we had a make_array function so we could write it as static const auto x = make_array<T>(...), so we dont have to update the size when we add a new element.

That would make be useful with enabling the modernize-avoid-c-arrays clang-tidy warning.

[firewave]

make_array is problematic since it requires varargs and the count of the varargs. Examples I looked at pass the count. So nothing won.

[chrchr-github]

We could switch to C++17 (or 20) and do this: https://godbolt.org/z/GjoPoa69o

[firewave]

We could switch to C++17 (or 20) and do this: https://godbolt.org/z/GjoPoa69o

We barely were able to raise the minimum requirement to GCC 4.8 and LLVM just recently switched to C++17. I guess that's still at least a decade off for us. 😁

[pfultz2]

make_array is problematic since it requires varargs and the count of the varargs. Examples I looked at pass the count. So nothing won.

I am not sure I follow the problem. You can get the count with sizeof...(Ts):

template<class T, class... Ts>
std::array<T, sizeof...(Ts)> make_array(Ts&&... xs)
{
    return {std::forward<Ts>(xs)...};
}

The problem is that we can't pass the initializer lists to this function, like make_array<std::pair<std::string, std::string>>({"==", "0"}, {"<", "0"}, {"==", "-1"}, {"<=", "-1"}). Instead, we probably need to use something like std::to_array, which we could implement in pure C++11:

// Like std::index_sequence in c++14
template <size_t...> struct index_sequence { using type = index_sequence; };
// Used to construct the sequence with O(log n) template instantiations
template <class, class> struct merge_sequence;

template <size_t... Xs, size_t... Ys>
struct merge_sequence<index_sequence<Xs...>, index_sequence<Ys...>>
    : index_sequence<Xs..., (sizeof...(Xs) + Ys)...> {};

template <size_t N>
struct make_index_sequence
    : merge_sequence<typename make_index_sequence<N / 2>::type,
                     typename make_index_sequence<N - N / 2>::type> {};

template <> struct make_index_sequence<0> : index_sequence<> {};
template <> struct make_index_sequence<1> : index_sequence<0> {};

template<class T, size_t N, size_t... Is>
std::array<T, N> to_array_impl(const T (&x)[N], index_sequence<Is...>)
{
    return {{x[Is]...}};
}

template<class T, size_t N>
std::array<T, N> to_array(const T (&x)[N])
{
    return to_array_impl(x, make_index_sequence<N>{});
}

We should be able to write to_array<std::pair<std::string, std::string>>({{"==", "0"}, {"<", "0"}, {"==", "-1"}, {"<=", "-1"}}).

[firewave]

We should be able to write to_array<std::pair<std::string, std::string>>({{"==", "0"}, {"<", "0"}, {"==", "-1"}, {"<=", "-1"}}).

I think that's overkill to save specifying a single digit the compiler will tell us about if it is wrong - and will probably never be touched again.

I will consider this if we ever apply the modernize-avoid-c-arrays fixes and it's an substantial amount of code affected.

[danmar]

hmm.. shouldn't we be able to construct the array without extra {}?

static const std::array<std::string, 5> suffixes{"", "u8", "u", "U", "L"};

[firewave]

We are, but not in all cases. I removed the unnecessary extra ones.