AMREX_ENUM: Cache String Map

[ax3l]

Summary

The getEnumNameValuePairs logic used below a lot of AMREX_ENUM functions is very slow for long-enough (e.g., N>70) enums.

In WarpX 1D simulations, it took up close to 50% of the runtime, as Dave Grote found.

This caches the key-value string list on first creation, because the type is known at compile-time.

A better implementation might skip the dictionary altogether for functions that just need one string, like getEnumNameString.

Additional background

BLAST-WarpX/warpx#6139

Checklist

The proposed changes:

• fix a bug or incorrect behavior in AMReX
• add new capabilities to AMReX
• changes answers in the test suite to more than roundoff level
• are likely to significantly affect the results of downstream AMReX users
• include documentation in the code and/or rst files, if appropriate

[ax3l]

I do not get what the original & was aiming to achieve here, because the getEnumNameValuePairs function returns by value.

[WeiqunZhang]

https://herbsutter.com/2008/01/01/gotw-88-a-candidate-for-the-most-important-const/

[ax3l]

Nice thx. TIL.

I think it still needs to go now that it is static (?)

[WeiqunZhang]

By the way, although I am not against using const, I use it less frequently than you. Anyway, always const is certainly wrong. Here is an example.

std::vector<int> make_vector()
{
     std::vector<int> const r(...);
     // code using r
     return r;
}

auto r = make_vector();

That const will prevent move, I think.

[ax3l]

That is correct. There is a similar case preferring by-value arg passing (over const &).

[WeiqunZhang]

I think we should create a helper function to avoid multiple statics. Something like

    template <typename T, typename ET = amrex_enum_traits<T>,
              std::enable_if_t<ET::value,int> = 0>
    std::vector<std::pair<std::string,T>> getEnumNameValuePairs_helper ()
    {
            // the function body of current getEnumNameValuePairs
    }

    template <typename T, typename ET = amrex_enum_traits<T>,
              std::enable_if_t<ET::value,int> = 0>
    std::vector<std::pair<std::string,T>> const& getEnumNameValuePairs ()
    {
        static auto r = getNumNameValuePairs_helper<T>();
        return r;
    }

[ax3l]

Can we even create an optimized version of getEnumNameString that avoids creating a list of all (unused) names altogether? @WeiqunZhang

[WeiqunZhang]

Depending the size, maybe using std::map or std::unordered_map is better than std::vector. But it's hard to avoid building the whole list.