Variant Based Pmts

[jsallay]

I was doing some benchmarking and noticed that some operations like object construction were significantly slower using the flatbuffers based pmts, then the old pmts. We aren't really using flatbuffers in the way they were intended. This started as an experiment in using an std::variant to try and speed things up.

It turns out that it does provide performance improvements and it simplifies the code significantly.

[RalphSteinhagen]

I like this PMT implementation that inherits from rva::variant which seems to be accepted in the upcoming C++23 standard (i.e. inheritance from std::variant STL should officially be safe by then).

Had a bit of time for reading and playing around with it. There is presently this rather long custom definition for:

using pmt_var_t = rva::variant<
    std::nullptr_t,
    uint8_t, uint16_t, uint32_t, uint64_t,
    int8_t, int16_t, int32_t, int64_t,
    float, double, std::complex<float>, std::complex<double>,
    std::vector<uint8_t>, std::vector<uint16_t>, std::vector<uint32_t>, std::vector<uint64_t>,
    std::vector<int8_t>, std::vector<int16_t>, std::vector<int32_t>, std::vector<int64_t>,
    std::vector<float>, std::vector<double>,
    std::vector<std::complex<float>>, std::vector<std::complex<double>>,
    std::string,
    std::vector<rva::self_t>,
    std::map<std::string, rva::self_t>>;

Unless certain combinations are intentionally suppressed (e.g. std::vector<std::string>) this definition could be made more generic and the related std::vector<T> , std::complex<T> and std::map<std::string, T> automatically generated for all supported types using a single parameter pack or via types stored in a std::tuple:

using pmt2_var_t = as_pmt_t<rva::variant, uint8_t, uint16_t, uint32_t, uint64_t, int8_t, int16_t, int32_t, int64_t, 
                                           float, double, std::string, rva::self_t>;

// initialisation via type list stored in tuple (N.B. tuple could be extended by user with custom OOT types)
using default_supported_types = std::tuple<uint8_t, uint16_t, uint32_t, uint64_t, int8_t, int16_t, int32_t, int64_t,
                                            float, double, std::string, rva::self_t>;
using pmt3_var_t = as_pmt_t<rva::variant, default_supported_types>; 

The latter tuple-based version would support user-defined types -- if this isn't supposed to be discouraged. For the corresponding helper templates

click here.

```cpp namespace detail { template class VariantType, typename... Args> struct as_pmt { using type = VariantType..., std::vector..., std::vector>..., std::map...>; }; template class TemplateType, typename ...T> struct as_pmt> { using type = as_pmt::type; }; } template class VariantType, class... Args> using as_pmt_t = detail::as_pmt::type; ```

See the corresponding compiler-explorer example for further details.

[RalphSteinhagen]

The using pmt_var_t definition above includes std::nullptr_t is this intentional/needed somewhere?

[RalphSteinhagen]

to note: until the rva::variant becomes superseeded by the C++23 standard, I'd recommend to add/include the license and original author to the variant header (+ deprecation note once moving to C++23) and maybe also the unit-tests.

I think including this directly rather than importing it as an external dependency is a fair-use. Especially since the source may not be maintained indefinitely.

Looking forward to this being merged. :+1:

[jsallay]

Thanks for the feedback. I originally had the parameter pack, but backed it out as I was trying to debug something. Agreed that it makes the code cleaner. I'll add it back it.

As far as allowing user-defined types, I am torn on that. Because pmts can be transmitted between different systems or created and consumed outside of gnuradio, we need to have consistent definitions. I am concerned that if we allow for user-defined types that we may end up with the problem of one system receiving a message and the other not being able to interpret it.

My present plan for user-defined structs is to use refl-cpp to convert them to a dictionary. I believe this is very similar to the IR that you use in the YAS. I'm open to other ideas.

The using pmt_var_t definition above includes std::nullptr_t is this intentional/needed somewhere?

The intention is to support the python-like syntax of my_dict["key"] = None and my_pmt == None. There are cases where it is important to communicate that a pmt has no value. It looks like std::monostate would probably be a better choice here. Would you agree?

to note: until the rva::variant becomes superseeded by the C++23 standard, I'd recommend to add/include the license and original author to the variant header (+ deprecation note once moving to C++23) and maybe also the unit-tests.

I think including this directly rather than importing it as an external dependency is a fair-use. Especially since the source may not be maintained indefinitely.

Agreed. I'll add in the licence and a note about being able to switch in C++23.

[RalphSteinhagen]

As far as allowing user-defined types, I am torn on that. Because pmts can be transmitted between different systems or created and consumed outside of gnuradio, we need to have consistent definitions. I am concerned that if we allow for user-defined types that we may end up with the problem of one system receiving a message and the other not being able to interpret it.

This requires that the user provides the same synchronised definition (unique ID number + serialiser specialisation) on both the sender and receiver sides. For YAS we adopted the policy that advanced users may extend by their own types. But I do understand if you want to limit this for simplicity/ease of maintenance/user support reasons.

My present plan for user-defined structs is to use refl-cpp to convert them to a dictionary.

That is also a valid choice. While we love and already heavily use refl-cpp, I didn't want to introduce this in GR 4.0 out of fear that some might consider this a step to much. If you integrate this, we'd be very happy and maybe this could be made then also available for the whole of GR 4.0? :grin:

It looks like std::monostate would probably be a better choice here. Would you agree?

Yes, std::monostate seems to be the better choice then. The 'nullptr' is often a bit dodgy (esp. in user-land, dereferencing, ...) and I prefer to avoid it in favour of RAII unless absolutely necessary.

Looking forward to this being merged.

P.S. I think we will also use rva::variant for our IoSerialiser implementation because we have some use-cases where we cannot have compile-time evaluated aggregates (e.g. other unknown/non-strongly-typed services, 'GRC' YML files, etc. :smirk: )

[jsallay]

I believe this is ready for review and merge. There are some unsigned commits. I tried to rebase and sign and that just created merge conflicts and more unsigned commits. I'm not really sure how to fix it.