picongpu icon indicating copy to clipboard operation
picongpu copied to clipboard

Replace mpl by mp11 (part 1/2)

Open bernhardmgruber opened this issue 2 years ago • 8 comments

This PR replaced most uses of boost::mpl by boost::mp11. Only boost::mpl::apply and the corresponding placeholders have not been replaced yet, because there is no direct equivalent in mp11. The functionality of mp11 is directly pulled into the pmacc namespace, since all mp11 type functions are prefixed with mp_.

  • [x] This PR requires at least Boost 1.66.0, because this is the first Boost version where Boost.Mp11 is available. Separate PR: #3939.
  • [x] This PR also makes use of C++17 features, so merging #3949 is a prerequisit.

bernhardmgruber avatar Sep 30 '21 14:09 bernhardmgruber

Looks definitely much shorter.

PrometheusPi avatar Sep 30 '21 21:09 PrometheusPi

Taking a time trace of this PR and the dev branch:

dev this PR
compilation time 68.1s 65.4s
instantiations 23648 16772
trace file size 74.1MiB 31.3MiB

I was disappointed that we could not win much in compilation speed. However, the amount of work in the frontend shrunk drastically. The amount of function and class template instantiations where reduced by around a third. This was also very visible in the size of the trace files.

clang frontend trace from dev: image

clang frontend trace from this PR: image

We can see a great reduction in the green vertical spike lengths, especially during parsing.

bernhardmgruber avatar Oct 15 '21 15:10 bernhardmgruber

Questions to move on:

  • How are users and developers expected to access mp11 functionality? Spelling boost::mp11::mp_.... is quite long. With mpl, we had the namespace alias bmpl. So far I have used an alias bmp for boost::mp11 in some places. But I think we could also just import mp11 directly into pmacc and picongpu since all metaprogramming functions are prefixed with mp_. The user could thus spell mp_... directly in any picongpu or pmacc namespace. What do you think?
  • Can we move to at least Boost 1.66? Should we split this off and merge it sooner?
  • There is no directy equivalent of MPL lambdas in mp11. That is, you cannot define a type function where several parameters are placeholders (e.g. _1) and then call something like apply<TypeFunction, Args>::fn which replaces the placeholders by actual template parameters. The workaround is to create alias templates and pass higher order type functions. In some cases this is more verbose, but faster to compile (no big impact though) and leads to less long error messages. We could also provide a similar facility on top of mp11. Currently, several places using mpl::apply with placeholders remain because they are not easy to transition. We should eventually move away from MPL completely (to not have 2 metaprogramming libraries). Can we merge this PR without finding a solution to MPL lambdas? Do we want to keep MPL for only its lambdas? Do we want a custom solution on top of mp11?
  • I replaced mpl::pair by a plain mp_list in some cases. For others, I created pmacc::meta::Pair. We would not need it though and could just use mp_list. mp11 does not have a dedicated pair type and this is fine IMO.
  • Should developers include mp11 headers directly, or should there be a central pmacc header pulling in mp11, that is supposed to be included by everyone else?
  • The PR is huge, should/can we split it in parts?
  • pmacc::CT::Vector could benefit from refactoring to use more constexpr instead of meta programming. The current version (with mp11) is not great, but this is not mp11's fault. Do anything here?
  • This PR requires C++17 already because of if constexpr in some places. Has PIConGPU switched already?

bernhardmgruber avatar Dec 08 '21 15:12 bernhardmgruber

Questions to move on:

  • How are users and developers expected to access mp11 functionality? Spelling boost::mp11::mp_.... is quite long. With mpl, we had the namespace alias bmpl. So far I have used an alias bmp for boost::mp11 in some places. But I think we could also just import mp11 directly into pmacc and picongpu since all metaprogramming functions are prefixed with mp_. The user could thus spell mp_... directly in any picongpu or pmacc namespace. What do you think?

Since pmacc is heavily depending on meta-programming I would be fine to pull mp11 into pmacc.

  • Can we move to at least Boost 1.66? Should we split this off and merge it sooner?

Yes would be possible but requires a separate PR. Why do you like to remove boost 1.65.1?

  • There is no directy equivalent of MPL lambdas in mp11. That is, you cannot define a type function where several parameters are placeholders (e.g. _1) and then call something like apply<TypeFunction, Args>::fn which replaces the placeholders by actual template parameters. The workaround is to create alias templates and pass higher order type functions. In some cases this is more verbose, but faster to compile (no big impact though) and leads to less long error messages. We could also provide a similar facility on top of mp11. Currently, several places using mpl::apply with placeholders remain because they are not easy to transition. We should eventually move away from MPL completely (to not have 2 metaprogramming libraries). Can we merge this PR without finding a solution to MPL lambdas? Do we want to keep MPL for only its lambdas? Do we want a custom solution on top of mp11?

We can not get rid of compile time lambdas. Most functionality depends on it. I am fine with going on with this PR without removing the mpl lambdas.

  • I replaced mpl::pair by a plain mp_list in some cases. For others, I created pmacc::meta::Pair. We would not need it though and could just use mp_list. mp11 does not have a dedicated pair type and this is fine IMO.

I am fine with pmacc::meta::Pair. Could we maybe use std::tuple. The code is from c++98 times, nowadays usage of std::tuple should be ok, but maybe I miss something because I have not looked into the usage.

  • Should developers include mp11 headers directly, or should there be a central pmacc header pulling in mp11, that is supposed to be included by everyone else?

I think if pmacc is pulling this dependency it is fine.

  • The PR is huge, should/can we split it in parts?

If possible I would prefer small PR and changing the parts step by step but since mpl is everywhere used it could be easier to change it within one PR.

  • pmacc::CT::Vector could benefit from refactoring to use more constexpr instead of meta programming. The current version (with mp11) is not great, but this is not mp11's fault. Do anything here?

I am fine to use constexpr where it makes sense and is useful. As I said above most code is from C++98 times and three was no way's around template magic where we could use now more elegant ways and therefore reduce the compile time too.

  • This PR requires C++17 already because of if constexpr in some places. Has PIConGPU switched already?

No PIConGPU is still C++14. We can switch the dev branch any time to C+17. The last time it was blocked by an issue in the dependency openPMD-api. Switching to C++17 requires a separate PR.

psychocoderHPC avatar Dec 09 '21 15:12 psychocoderHPC

Since pmacc is heavily depending on meta-programming I would be fine to pull mp11 into pmacc.

Done. All functionality is available as pmacc::mp_....

Why do you like to remove boost 1.65.1?

Boost.Mp11 is available since Boost 1.66. PR here: #3939

We can not get rid of compile time lambdas. Most functionality depends on it. I am fine with going on with this PR without removing the mpl lambdas.

OK!

I am fine with pmacc::meta::Pair. Could we maybe use std::tuple. The code is from c++98 times, nowadays usage of std::tuple should be ok, but maybe I miss something because I have not looked into the usage.

pmacc::meta::Pair is just a type list of fixed length 2. It's literally just:

    template<typename First, typename Second>
    struct Pair
    {
        using first = First;
        using second = Second;
    };

I think if pmacc is pulling this dependency it is fine.

I created a new header pmacc/meta/Mp11.hpp that pulls in Boost.Mp11.

We can switch the dev branch any time to C+17. The last time it was blocked by an issue in the dependency openPMD-api. Switching to C++17 requires a separate PR.

Can I take this as: Someone else is already working on that?

bernhardmgruber avatar Dec 09 '21 16:12 bernhardmgruber

About MPL Lambdas: I had a closer look on how they work and how this is solved in Mp11. In Mp11, mp_bind with placeholders is used, producing a quoted metafunction, instead of an MPL placeholder expressions and mpl::apply. In both cases the result is a type that can be put into type lists or passed as normal template argument.

Now, using mp_bind on the client side of APIs is probably less elegant. However, with some refactoring we can also eliminate placeholder expression entirely in many cases by passing type functions directly. For example most of meta::ForEach will be simplified from:

meta::ForEach<FileOutputFields, GetFields<boost::mpl::_1>> ForEachGetFields;
                        ForEachGetFields(threadParams);

to:

meta::ForEach<FileOutputFields, GetFields> ForEachGetFields;
                        ForEachGetFields(threadParams);

This is also reflected in Mp11, where many meta functions have a normal version accepting a meta function and a version accepting a quoted meta function (see _q suffixes).

What is still less elegant is transforming this from MPL:

using UnspecializedSpeciesPlugins = pmacc::mp_list<
    plugins::multi::Master<EnergyParticles<boost::mpl::_1>>,
    plugins::multi::Master<CalcEmittance<boost::mpl::_1>>,
    plugins::multi::Master<BinEnergyParticles<boost::mpl::_1>>,
    CountParticles<boost::mpl::_1>,
    PngPlugin<Visualisation<boost::mpl::_1, PngCreator>>,
    plugins::transitionRadiation::TransitionRadiation<boost::mpl::_1>
>;

With Mp11 bind:

using UnspecializedSpeciesPlugins = pmacc::mp_list<
    plugins::multi::Master<pmacc::mp_bind<EnergyParticles, pmacc::_1>>,
    plugins::multi::Master<pmacc::mp_bind<CalcEmittance, pmacc::_1>>,
    plugins::multi::Master<pmacc::mp_bind<BinEnergyParticles, pmacc::_1>>,
    pmacc::mp_bind<CountParticles, pmacc::_1>,
    PngPlugin<pmacc::mp_bind<Visualisation, pmacc::_1, PngCreator>>,
    pmacc::mp_bind<plugins::transitionRadiation::TransitionRadiation, pmacc::_1>
>;

If we want to support the original placeholder expressions (but using Mp11 placeholders), we could write our own meta function to evaluate it. The implementation is similar to mp_bind and probably in the order of 50-100 LOCs.

Ultimately, we are going to need to look at the various cases and apply local judgement. But I believe we can solve it and make it look nice :) In a separate PR of course.

bernhardmgruber avatar Dec 10 '21 07:12 bernhardmgruber

We can implement the placeholder expressions ourselves and I am considering that for LLAMA as well now. The implementation is rather simple: https://github.com/alpaka-group/llama/pull/451/files

bernhardmgruber avatar Dec 22 '21 14:12 bernhardmgruber

We can implement the placeholder expressions ourselves and I am considering that for LLAMA as well now. The implementation is rather simple: https://github.com/alpaka-group/llama/pull/451/files

Ohh that's cool. I will check it next year^^

psychocoderHPC avatar Dec 22 '21 19:12 psychocoderHPC

I have this PR on my radar and will try to rebase and test it together with #3983 soon.

psychocoderHPC avatar Mar 02 '23 16:03 psychocoderHPC

I will test this tomorrow on crusher and merge it if all works as expected.

psychocoderHPC avatar Mar 28 '23 16:03 psychocoderHPC

@bernhardmgruber Thanks for your hard work!

psychocoderHPC avatar Mar 29 '23 13:03 psychocoderHPC