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

Replace mpl by mp11 (part 1/2) #3834

Merged

Conversation

bernhardmgruber
Copy link
Contributor

@bernhardmgruber bernhardmgruber commented Sep 30, 2021

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_.

  • This PR requires at least Boost 1.66.0, because this is the first Boost version where Boost.Mp11 is available. Separate PR: Upgrade to Boost 1.66 #3939.
  • This PR also makes use of C++17 features, so merging Switch to C++17 #3949 is a prerequisite.

[update psychocoderHPC] This PR is making old input parameter sets incompatible due to the switch from boost::mpl to boost::mp11::mp_list.

@PrometheusPi
Copy link
Member

Looks definitely much shorter.

@bernhardmgruber bernhardmgruber changed the title Rewrite meta::ForEach using boost::mp11 Replace mpl by mp11 Oct 15, 2021
@bernhardmgruber
Copy link
Contributor Author

bernhardmgruber commented Oct 15, 2021

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.

Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please rebase this PR and squash the changes, maybe add all .param changes into one commit, this would help to see what the user must change to be able to run old setups with the new required changes.

@bernhardmgruber
Copy link
Contributor Author

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?

@psychocoderHPC
Copy link
Member

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.

@bernhardmgruber
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

Copy link
Member

@psychocoderHPC psychocoderHPC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes are required and we need to solve the warnings created by meta::ForEach

include/pmacc/meta/ForEach.hpp Outdated Show resolved Hide resolved
include/picongpu/plugins/IsaacPlugin.hpp Outdated Show resolved Hide resolved
include/picongpu/plugins/common/openPMDWriteMeta.hpp Outdated Show resolved Hide resolved
include/pmacc/math/vector/compile-time/Size_t.hpp Outdated Show resolved Hide resolved
include/pmacc/math/vector/compile-time/UInt32.hpp Outdated Show resolved Hide resolved
include/pmacc/math/vector/compile-time/UInt64.hpp Outdated Show resolved Hide resolved
include/pmacc/particles/memory/dataTypes/Particle.hpp Outdated Show resolved Hide resolved
bernhardmgruber and others added 2 commits March 22, 2023 16:26
- shared memory: fix type deduction
- workaround nvcc bugs
@psychocoderHPC
Copy link
Member

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

@psychocoderHPC
Copy link
Member

@bernhardmgruber Thanks for your hard work!

@psychocoderHPC psychocoderHPC merged commit 5ff83cd into ComputationalRadiationPhysics:dev Mar 29, 2023
@psychocoderHPC psychocoderHPC added the component: user input signals changes in user API such as .param files, .cfg syntax, etc. - changelog! label Mar 29, 2023
@bernhardmgruber bernhardmgruber deleted the mp11 branch March 29, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: PMacc in PMacc component: user input signals changes in user API such as .param files, .cfg syntax, etc. - changelog!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants