stephenberry / glaze

Extremely fast, in memory, JSON and interface library for modern C++
MIT License
1.12k stars 113 forks source link

How to deal with inherited structs? #178

Closed fregate closed 1 year ago

fregate commented 1 year ago

Hi! I'm very exited about performance for encoding of this application and want to ask about how to use it in case like

class A
{
    int a;
    double b;

public:
    int a() const { return a; }
    double b() const { return b; }
};

template <>
struct glz::meta<A>
{
    static constexpr auto value = glz::object(
        "a", [](auto && v) { return v.a(); }, 
        "b", [](auto && v) { return v.b(); }
    );
};

class B : public A
{
    std::string c;

public:
    const std::string & c() const { return c; }
};

template <>
struct glz::meta<B>
{
    static constexpr auto value = glz::object("c", [](auto && v) { return v.c(); }); // add everything from A?
};

class Outer
{
    std::uinque_ptr<A> ptr;

public:
    Outer() { ptr = std::make_unique<B>(); }
    const std::uinque_ptr<const A> ptr() const { return ptr; }
}

template <>
struct glz::meta<Outer>
{
    static constexpr auto value = glz::object("ptr", [](auto && v) { return v.ptr(); }  //  ????
    );
};

int main(int, char **)
{
    Outer o;
    std::cout << glz::write_json(o);
    return 0;
}

What is the correct way to do this inheritance thing? In fact I have abstract class-interface and got errors like:

write.hpp:646:63: error: cannot allocate an object of abstract type ‘MessagePayload’
common.hpp:749:30: error: cannot allocate an object of abstract type ‘MessagePayload’

I tried to output json string, but in result string I see string, not object (of course) "payload":"{\"num\":1}" - and this is not correct for me. In tests I can't find any related cases.

fregate commented 1 year ago

Make it variants? Like this std::variant<var1_t, var2_t> v{}; But in that way I have make all my structs visible. If this only way, ok, but maybe there is another?

fregate commented 1 year ago

Ok. I've found another solution, make glz::json_t json() override in derived classes, and do something like

return {
    {"num", number_}
};

But in wiki you said that is not optimal solution because of memory allocations.

Is there is way to get json_t from already defined struct glz::meta<MyDerivedClass>? And not build it from scratch in json function, like return *this or return glz::meta<MyDerivedClass>(this)?

mwalcott3 commented 1 year ago

I'm going to chime in with my opinion on the matter but its really up to @stephenberry so Ill talk to him later about if and how we want to handle inheritance and polymorphism. Much like the standard library we tend to rely more on templates rather than polymorphism hence why we haven't addressed this until now. Note that it is quite likely we might not add support for this period because there are a lot of problems that have to be worked out.

Inheritance is not currently supported but wouldn't be that hard to handle. Polymorphism on the other hand is a massive pain since glaze relies heavily on knowing the type it is dealing with so It would need to be able to do visitation which would require knowing the types. Luckily we don't need to list all the types in one place like in a variant and we can leverage either a compile time or runtime type registry. I lean towards compile time but there are benefits to a runtime registry.

In my proposed implementation after the meta struct you would need to specify what classes it inherits from. If we want a compiletime registry there isn't really a good way of adding such a thing to glz::meta and the code to set it up is quite boilerplatey so unfortunately it would need to use macros. In one possible implementation If B, C, and D all inherited from A you would need to put the following after each respective glz::meta.

//Something like this
//B meta
GLZ_INHERITANCE(B, A);
//C meta
GLZ_INHERITANCE(C, A);
//D meta
GLZ_INHERITANCE(D, A);

And for full support they would likely all need names added to the metas (static constexpr std::string_view name = "B";)

Alternatively we could add the base class to the meta and then just register the type:

//Something like this
template <>
struct glz::meta<B>
{
        using base = A; // We could handle multiple inheritance by adding something like glz::type_list
        static constexpr std::string_view name = "B";
    static constexpr auto value = glz::object("c", [](auto && v) { return v.c(); });
};
GLZ_REGISTER(B);

The benefit of this is that we could add support for handling any registered object type in a std::any automatically and would be helpful if we wanted to add support for stuff like generating language bindings. This approach also wouldn't require registration unless polymorphism was used. I lean towards this approach. I feel like this will also work better when we have static reflection.

Assuming we can agree on an approach while its not supper simple this can probably be handled in under a week.

We would then be able to automatically handle inheritance/polymorphism for registered types.

Here is some code I was playing around with to test out compiletime type registration. https://godbolt.org/z/MhasjrWo4

The errors you saw are likely another issue in which there are various sections of code that assume things are default constructible and we really should get rid of those.

Another thing to point out in the code you posted is that since the lamdas in the glz::object are returning values they would only support writing out to json and not reading in. I think @stephenberry was planning on adding setter/getter support since it was the most general way of supporting bitfields but its not a pattern we normally use. I'm surprised noone has run into this yet.

Is there is way to get json_t from already defined struct glz::meta? And not build it from scratch in json function, like return *this or return glz::meta(this)?

I'm sorry I don't quite understand.

fregate commented 1 year ago

I'm sorry I don't quite understand.

I asked about - is there is way to obtain json_t (or smthing like that) from meta and not build it in function from scratch?

Another thing to point out in the code you posted is that since the lamdas in the glz::object are returning values they would only support writing out to json and not reading in

Yeah, I understood. At this moment I need to write jsons only, not read. So for me it's Ok.

fregate commented 1 year ago

Anyway, thank you for quick response!

mwalcott3 commented 1 year ago

I asked about - is there is way to obtain json_t (or smthing like that) from meta and not build it in function from scratch?

No unfortunately not. Ill keep it in mind though.

Ill try to add the inheritance/polymorphism stuff in soon provided I get approval. Hopefully that will address your use case.

mwalcott3 commented 1 year ago

We might also be able to handle this through having the user supply a read/write function in the base class that is called and takes in the iterators for the buffer and the context. The derived class can then call write/read with the correct type. I didn't originally think of this because it doesn't allow deducing the type when reading and it would have to read to whatever type is currently stored but we cant entirely support changing the underlying type unless its being stored in a std::unique_ptr or std::sharered_ptr anyways (And the user may not want to let us change the type).

I still prefer registering the type since it meshes better with the rest of the lib and I can anticipate other use cases. The only problems with the type registration is that it requires rtri (We just got rid of all rtii usage in glaze) and writing might require a linear search since typeids are not constexpr we would need to build a runtime mapping if we didn't want to do a linear search. Still way more performant than relying on something like json_t. Reading would be fine though since we wouldn't need to depend on rtti. A polymorphic write function would be much faster......

Edit: Note that while we can't get rid of the rtti req if using registration to handle polymorphism we can get rid of the linear search on all major platforms. While it is not guaranteed by the standard the pointers to the type info are generally all you need for an equality check (https://gcc.gnu.org/onlinedocs/gcc-4.6.2/libstdc++/api/a01094_source.html) and while we can't get the typeid in a constexpr manner we can get its pointer. We can build a compiletime perfect hash table and do the lookup using that. Then perform the full equality check to make sure and if that fails use a linear search (On most platforms this should not be needed). Still slower than using a polymorphic function call but this should be more acceptable than a linear search when you have many potentail types.

mwalcott3 commented 1 year ago

I asked about - is there is way to obtain json_t (or smthing like that) from meta and not build it in function from scratch?

I dislike json_t but I think we should add a way of doing this since like you pointed out we have the structure information from the meta so as long as two things share a similar structure (And json_t should match any structure) we can generate the code to copy the data out. Something like "auto json = glz::convert(*this);" So I opened up a new issue #179 to track that separately and we can handle inheritance here.

Also, we should probably add pmr support to address some of the performance concerns in json_t.

mwalcott3 commented 1 year ago

I talked with @stephenberry and we don't want to rush into inheritance/polymorphism. We don't want to be adding major features right now. Especially since we don't tend to use allot of polymorphism ourselves. We will likely initially just go with your suggestion of creating a json_t from a meta type for now as a stop gap as it is generally useful and straightforward.

There are concerns with build times when using a compile time type registry to build the inheritance hierarchy and we don't want to use rtti but need something similar if we want to handle deduction. Like a virtual function that returns a glaze type hash if rtii is turned off. And there are concerns about how this would work across dll boundaries if we did use rtii. The biggest concern is that other potential enhancements like better custom read/write might overlap with this issue so we just want to wait and see for now.

@fregate, I dont like leaving people hanging but we are unlikely to improve support in this area in the short term

I tried to output json string, but in result string I see string, not object (of course) "payload":"{\"num\":1}" - and this is not correct for me.

You could use the glz::raw_json (Undocumented) type to get around this the contained string will be treated as raw json and will not be output as a string. https://github.com/stephenberry/glaze/blob/2052a5d32c8297fe7abcf4ab365190418217afe6/include/glaze/core/common.hpp#L183 Would likely be faster than dealing with json_t

Make it variants? Like this std::variant<var1_t, var2_t> v{}; But in that way I have make all my structs visible.

Yeah a variant of pointers to your derived types should work but would require you to know all the potential types. Probably what I would have tried as a workaround due to the fact it should handle reading/writing but I understand its not ideal.

fregate commented 1 year ago

Ok. Thank you for response and new features to implement in future. I will try with glz::raw_json to understand how it works. And of course, follow this project.

Anyway, I made some changes to compile it with gcc-10 (for my embedded-like project). Is it interesting or you targeting only for new compilers and standards?

mwalcott3 commented 1 year ago

Anyway, I made some changes to compile it with gcc-10 (for my embedded-like project). Is it interesting or you targeting only for new compilers and standards?

I don't think we were motivated enough to update glaze to support gcc10 but it sounds like you already did the heavy lifting. If you make a pull request or publish your fork on github (didnt see it) I'll review the changes and merge them provided everything looks good. I'll set up a ci build for gcc10 but I can't promise we won't drop support for it later if it's missing something we want to use.

mwalcott3 commented 1 year ago

No macro solution to the type registry problem https://godbolt.org/z/onYTjhf66. Or maybe something like this https://godbolt.org/z/d8s3cK1rE. I think I prefer using static_asserts like this this https://godbolt.org/z/ncvGxxc5Y

This essentially allows us to do the same things QT does in its meta system with qRegisterMetaType but compiletime instead of runtime.

This is one half of the things required to handle polymorphism cleanly. A constexpr typeid paired with some sort of runtime typeid is the second (Either through rtii, a polymorphic function call, or if there was some standard way of getting the vtable pointer I wouldn't even need that).