Plans for cppgraphqlgen 4.0

[wravery]

This is where I'm going to start tracking features for 4.0, the next major version of cppgraphqlgen. This will be a breaking change for existing consumers, so it's an opportunity to cleanup deprecated methods and simplify some of the APIs. I'm also going to adopt C++20 features if they are well enough supported on Windows, Linux, and macOS.

• Stand up a branch for 4.0 and start making PRs against that: https://github.com/microsoft/cppgraphqlgen/tree/next.
• C++20: Co-routines Implemented in https://github.com/microsoft/cppgraphqlgen/tree/next.
• Finer grained asynchrony Substitute a custom awaitable instead of passing a std::launch enum value to allow plugging in a custom thread pool for example.
• C++20: Concepts In progress, used for type-erasure (see below), but still more use cases to consider.
• Type Erasure instead of inheritance Implemented in https://github.com/microsoft/cppgraphqlgen/tree/next. Solves const methods prevent storing context in subclasses #171 and makes the design much more decoupled. Depends on C++20 Concepts for the conditional handling of stubs which may be unimplemented on the target type.
• Use specific types for interfaces and unions instead of service::Object Generate type-erased objects for interface types, and return std::variant<> for the options in a union.
• Simplified code gen by removing options or making new defaults (e.g. --no-stubs by default, type erased headers instead of inheritance, etc.).
• Add CMake functions to module config which help with codegen targets (Unable to use cppgraphqlgen #170) Particularly schemagen --separate-files. Might be able to make --separate-files the only option to simplify schemagen and improve incremental build times.
• Otherwise modernize CMake Bump the minimum required version as necessary, remove fallbacks and global config variables for earlier versions. Partially implemented in https://github.com/microsoft/cppgraphqlgen/tree/next.
• Make separate .cpp files include only the ...Object.h headers that they reference in their return types. Tweak headers to make compilation faster #156
• Update to the recently released https://spec.graphql.org/October2021/ version of GraphQL itself.
• Export a target for RapidJSON internal details Could be done with a minor version update, but should be implemented in 4.0 at the latest: Export writeResponse and SubscriptionKey based delivery #169.
• Allow returning std::shared_ptr<T> from getters and avoid/defer memory copies in response::Value See discussion below. Type-Erasure can make this cleaner since you can actually make that the return type of the getter, but it should work in 3.0 as well where the getters all return service::FieldResult.
• Update docs

I've decided to postpone adopting these C++20 features because support is still uneven, and several of them require using compiler flags to enable them:

• C++20: Modules Pending, requires support in CMake, should make compilation of templates faster.
• C++20: Ranges Pending, should modernize a lot of begin/end iterator usage.
• C++20: std::fmt instead of std::ostringstream Pending, may require update to MSVC for standards errata.

This proved too complicated. Since FieldResult (now AwaitableScalar/AwaitableObject) uses the modified type wrapped in std::vector and std::optional, it can't just wrap the result in a known service::Object type-erased wrapper. To make this work, the field accessor return type would need to be parameterized with all of the same modifiers as ModifiedResult instead of the C++ type, which would make deciphering the correct type to use in a field accessor implementation much harder:

• Implicitly convert to type-erased objects in service::FieldResult We should be able to just return the std::shared_ptr<T> and let service::FieldResult wrap it, since it knows the expected type-erased object type. Even better, could it implicitly handle conversions of the value_type in std::vector or std::optional, and nest the convertible types that way? The logic in service::ModifiedResult seems to suggest it's possible.

I'll update this list as new features crop up and as I/we make progress on it.

[gitmodimo]

I have few suggestions for next version (apart from previously mentioned #169 #171). While implementing solution based on this library I can see few places where we can do better.

Major changes:

1. Allow returning values by reference.
   For example if we have big array of integers and we want to serve it with current approach we have to copy the array each time. If we allow returning references eg. by expanding variant within FieldResult to hold shared_ptr/weak_ptr/reference (or even access functor), we can skip the copy and gain significant performance boost.
2. GraphQL defines custom scalar types. It would be very useful for user to allow creation custom scalar types in cppgraphqlgen. I see this implemented as abstract subtype of BaseType with all methods needed to convert it to and back from Json. This will remove a lot of tedious coding of implementing a big structure that is always used as whole anyway.

Minor changes:

3. SubscriptionKey should be available in ResolverContext::Subscription, ResolverContext::NotifySubscribe and ResolverContext::NotifyUnsubscribe contexts by default. For now I extended context to hold this value.
4. Events ResolverContext::NotifySubscribe and ResolverContext::NotifyUnsubscribe should be issued regardless of launch policy. This change of behaviour is very misleading. Alternatively provide separate method for NotifySubscribe/Unsubscribe.

I can do some groundwork on first two proposals since I need them anyway. However it would be a waste if they would not be used/implemented another way. @wravery what is you view on all this?

EDIT:
On the second thought 2. does not seem to be feasible since PEG needs to understand syntax of custom type?

[wravery]

1. Allow returning values by reference.
   For example if we have big array of integers and we want to serve it with current approach we have to copy the array each time. If we allow returning references eg. by expanding variant within FieldResult to hold shared_ptr/weak_ptr/reference (or even access functor), we can skip the copy and gain significant performance boost.

I'll need to think about how this could be generalized some more. At some point, it needs to effectively copy or move the value into the response document, and eventually it needs to be converted to a response::Value and typically serialized to a JSON string buffer. Returning by value at least allows the construction of the response::Value to use move semantics and avoid an implicit copy there.

I have another experimental branch where I've been using type erasure instead of inheriting from the generated schema types: https://github.com/microsoft/cppgraphqlgen/tree/next. This already solves #171, since the getter signatures are no longer inherited. They just need to satisfy a concept which says that you can call a method with that name and implicitly convert any parameters that were passed, and the return type needs to be explicitly convertible to service::FieldResult. You don't need to match the exact return or argument types, or the const/noexcept specifiers. It's still very experimental, but I think if we want to enable flexible return types like references, it should probably be built on top of this.

2. GraphQL defines custom scalar types. It would be very useful for user to allow creation custom scalar types in cppgraphqlgen. I see this implemented as abstract subtype of BaseType with all methods needed to convert it to and back from Json. This will remove a lot of tedious coding of implementing a big structure that is always used as whole anyway.

EDIT: On the second thought 2. does not seem to be feasible since PEG needs to understand syntax of custom type?

Yes, custom scalars are opaque to the schema. There are a couple of examples in the GraphQL spec, of things like geolocation coordinates or a complex DateTime type, but it's up to the service and client implementations to use consistent conventions when handling them.

3. SubscriptionKey should be available in ResolverContext::Subscription, ResolverContext::NotifySubscribe and ResolverContext::NotifyUnsubscribe contexts by default. For now I extended context to hold this value.

Makes sense. 👍🏼

4. Events ResolverContext::NotifySubscribe and ResolverContext::NotifyUnsubscribe should be issued regardless of launch policy. This change of behaviour is very misleading. Alternatively provide separate method for NotifySubscribe/Unsubscribe.

Yes, this grew out of maintaining backwards compatibility with the original synchronous APIs, I've been waiting for 4.0 to clean up the disparity. I'm also planning on replacing the launch policy with a custom awaitable (with defaults that behave as before), so it should be possible for consumers to plug in their own thread pools or task engine.

I can do some groundwork on first two proposals since I need them anyway. However it would be a waste if they would not be used/implemented another way. @wravery what is you view on all this?

You're welcome to work on any of these 😄, but let me merge some of my progress in my personal fork with a new branch in the upstream repo first. Then we'll have a common base commit. I'll add a task to the list for this issue so you know when it's ready. Done: https://github.com/microsoft/cppgraphqlgen/tree/next

[wravery]

I have another experimental branch where I've been using type erasure instead of inheriting from the generated schema types: https://github.com/microsoft/cppgraphqlgen/tree/next. This already solves #171, since the getter signatures are no longer inherited. They just need to satisfy a concept which says that you can call a method with that name and implicitly convert any parameters that were passed, and the return type needs to be explicitly convertible to service::FieldResult. You don't need to match the exact return or argument types, or the const/noexcept specifiers. It's still very experimental, but I think if we want to enable flexible return types like references, it should probably be built on top of this.

It turns out this already works in this branch. 😄 Since service::FieldResult uses perfect forwarding in a template constructor, it's able to construct its _value member with either a const reference or an r-value reference and move semantics.

Although the service::FieldResult will still perform a copy in this case, it makes the getters a lot more reusable, especially now that the service::FieldParams argument is optional. For instance, I was able to remove and replace a bunch of redundant accessor methods in the star_wars directory:

const response::StringType& id() const noexcept;

with calls directly to the simplified getter which now has the same signature:

const response::StringType& getId() const noexcept;

[gitmodimo]

I have another experimental branch where I've been using type erasure instead of inheriting from the generated schema types: https://github.com/microsoft/cppgraphqlgen/tree/next.

I tried compiling it, however at this point I am stuck at gcc 10.2 which does not seem to cope with coroutines :( .
GraphQLService.cpp:1878:1: internal compiler error: in captures_temporary, at cp/coroutines.cc:2799

For now I will stick to version 3.

At some point, it needs to effectively copy or move the value into the response document, and eventually it needs to be converted to a response::Value and typically serialized to a JSON string buffer.

I tried profiling my use case to verify copying is the issue. You are correct. I think I now better understand how all of this works. Copying is not the bottleneck. Building response document is. It is extremely expensive. Since usually we only convert it to JSON maybe we skip the step of building this document? Or at least enable such approach.
The way I see it at this point (I am still new to all this codebase) is we create SAXVisitor (functionally similar to SelectionVisitor) that takes as template argument generic SAX interface (ex. like one in RapidJson) and instead of building response invokes relevant SAX methods (beginObject, endObject, Key, Int etc.).
That way we skip building response document and build JSON response on the fly very efficiently. Additionally JSON implementation is nicely separated by SAX interface common to all JSON libraries.

[wravery]

I tried profiling my use case to verify copying is the issue. You are correct. I think I now better understand how all of this works. Copying is not the bottleneck. Building response document is. It is extremely expensive. Since usually we only convert it to JSON maybe we skip the step of building this document? Or at least enable such approach. The way I see it at this point (I am still new to all this codebase) is we create SAXVisitor (functionally similar to SelectionVisitor) that takes as template argument generic SAX interface (ex. like one in RapidJson) and instead of building response invokes relevant SAX methods (beginObject, endObject, Key, Int etc.). That way we skip building response document and build JSON response on the fly very efficiently. Additionally JSON implementation is nicely separated by SAX interface common to all JSON libraries.

The response::toJSON function in the default RapidJSON version of graphqljson does use a SAX interface to perform the serialization. We never actually build a full JSON DOM using RapidJSON. The role of response::Value is to be a higher level abstraction of a JSON value, since GraphQL doesn't actually specify that the results are JSON. I'm not even sure the spec requires that they are serializable, which is something that clientgen can take advantage of (in-process, or with shared memory) to convert response::Value to strongly typed DTOs without any JSON library involved.

I think if we want to completely avoid copying, and the implementer is sure that they can keep the values alive long enough to return references, then there might be a way to let response::Value and service::FieldResult hold onto additional std::variant types for std::cref<T> and defer copying until it reaches response::toJSON. It would need to be plumbed through a lot of template code which expects return-by-value and move semantics, though. It would also be almost impossible to reason about the response::Value lifetime, so to be safe it should really use std::shared_ptr<T>. Even though that adds some of its own overhead, it's the only way to prevent a mess of dangling references.

This would be a very big change indeed, but it may not even be a breaking change. If it's integrated at the service::FieldResult level, so getters can start returning std::shared_ptr<T> and implicitly constructing the service::FieldResult from that, then I think it would work in 3.0 as well. I'll add it to the list for consideration in 4.0, and then if it works, maybe we could backport it to a final minor version release of 3.0.

[gitmodimo]

The role of response::Value is to be a higher level abstraction of a JSON value, since GraphQL doesn't actually specify that the results are JSON.

I now understand how it works. I must say all this is very cleverly thought through.

I think if we want to completely avoid copying, and the implementer is sure that they can keep the values alive long enough to return references, then there might be a way to let response::Value and service::FieldResult hold onto additional std::variant types for std::cref and defer copying until it reaches response::toJSON. It would need to be plumbed through a lot of template code which expects return-by-value and move semantics, though. It would also be almost impossible to reason about the response::Value lifetime, so to be safe it should really use std::shared_ptr. Even though that adds some of its own overhead, it's the only way to prevent a mess of dangling references.

I also narrowed down the performance hit I receive. Consider big vector of scalar types. During conversion it explodes in to vector of futures (each validating the same wrappedParams) and each element is then stored in Value.

It seems a way forward for me is to define custom scalar type in schema and use it instead of this big array. To do so I need to extend Value type to hold my arbitrary data and provide accessor for it. My first idea is to extend variant and allow it to store some abstract std::unique_ptr<CustomScalarBase>. Then it is up to user what is stored inside of it, whether it is reference or shared_ptr.

EDIT:
I tried and it fixed all my performance issues. Here is relevant commit in my fork. This generally allows putting custom types in Value. I am not sure this can be easily extended for arguments? Diving into PEG is beyond me at this point.
In my current solution I am abusing this new custom type to handle my array as a whole in order to gain performance. Maybe future version could recognise 'nondivisible' types like arrays(ones that query cannot dissect it in any way) and generate code to handle them as a whole?

[wravery]

EDIT: I tried and it fixed all my performance issues. Here is relevant commit in my fork. This generally allows putting custom types in Value. I am not sure this can be easily extended for arguments? Diving into PEG is beyond me at this point. In my current solution I am abusing this new custom type to handle my array as a whole in order to gain performance. Maybe future version could recognise 'nondivisible' types like arrays(ones that query cannot dissect it in any way) and generate code to handle them as a whole?

I think there are a couple of ways to handle this:

• Define pure-virtual methods on CustomTypeBase to implement toJSON and parseJSON, or simpler,
• Replace CustomType with std::shared_ptr<response::Value>.

Using a std::shared_ptr<response::Value> would let all of the existing code work, it would just need to dereference the shared_ptr. Instead of keeping a type that needs to be transformed eventually into a response::Value and returning that from the getter, the implementer could build and cache the result in a std::shared_ptr<response::Value> member and hand out copies of the shared_ptr.

It should be pretty straightforward to implement this for scalars since they already return a response::Value and convert it with service::ModifiedResult<>. Making other return types take either a std::shared_ptr or the embedded type might be trickier (again, helped by flexible return types in 4.0 with type erasure).

[wravery]

I tried compiling it, however at this point I am stuck at gcc 10.2 which does not seem to cope with coroutines :( . GraphQLService.cpp:1878:1: internal compiler error: in captures_temporary, at cp/coroutines.cc:2799

@gitmodimo , if you can get a minor version upgrade, it looks like this works in gcc 10.3.0. That's what the Linux CI build uses on the next branch:

-- The C compiler identification is GNU 10.3.0
-- The CXX compiler identification is GNU 10.3.0

[gitmodimo]

@gitmodimo , if you can get a minor version upgrade, it looks like this works in gcc 10.3.0. That's what the Linux CI build uses on the next branch:

Unfortunately it is too big of undertaking. I am using precompiled set of tools from vendor. The tool are targeting aarch64 and at this point replacing toolchain is not viable.

I think there are a couple of ways to handle this:
Define pure-virtual methods on CustomTypeBase to implement toJSON and parseJSON, or simpler,
Replace CustomType with std::shared_ptrresponse::Value.

I went with virtual function approach. I defined my CustomType abstract class (outside of library):

struct CustomType:public graphql::response::CustomTypeBase{
	virtual void writeJson(rapidjson::Writer<rapidjson::StringBuffer>& writer) const=0;
};

And then I implemented my custom type holding array of doubles as a reference:

struct floatArray:public webgui::CustomType{
	floatArray(const std::vector<double> &_vector):vector(_vector){

	}

	void writeJson(rapidjson::Writer<rapidjson::StringBuffer>& writer) const override{
		writer.StartArray();
		for(auto d:vector)
			writer.Double(d);
		writer.EndArray();
	}
	const std::vector<double> &vector;
};

Then in a getter I return return response::Value{std::make_unique<floatArray>(line.x)};.
All this works very efficiently. Of course I had to reimplement writeResponse to handle my custom types:

case Type::Custom:
		{
			auto custom = response.release<graphql::response::CustomType>();
			auto ptr=static_unique_pointer_cast<CustomType>(std::move(custom));
			ptr->writeJson(writer);
			break;
		}

Not sure how much of this fits inside of the library. At this point I tried to keep library modifications to minimum. This seems flexible enough to handle all unusual cases.

Currently in my application I rely on two changes Value storing virtual CustomTypeBase and SubscriptionKey based delivery. I would really appreciate if future version of the library would incorporate equivalent functionalities (SubscriptionKey based delivery is already on todo list).