Open audunhalland opened 8 months ago
Sweet, this looks great! I don't have time to look at this right now but perhaps @tyranron has opinions.
@LegNeato this is definitely on my agenda in near future. However, the diif is huge enough, which will take some time.
@audunhalland I'm going to review this in next few days and release in 0.17.0.
@tyranron cool, I'm not 100% happy with all parts of this PR, there are surely things that can be iterated on later. Especially I see DynType
and AsDynType
as a kind of hacky way to limit the scope and get it over the line.
edit: also note for the review: I'm not sure I understand the reflect
module fully, like e.g.:
impl<S> reflect::WrappedType<S> for ArcStr {
const VALUE: reflect::WrappedValue = 1;
}
@audunhalland
edit: also note for the review: I'm not sure I understand the
reflect
module fully, like e.g.:impl<S> reflect::WrappedType<S> for ArcStr { const VALUE: reflect::WrappedValue = 1; }
That should be fine, yes.
reflect::WrappedType::VALUE
is a tricky way to encode composed GraphQL types in prime numbers, since we're kinda limited with const
evaluation abilities in Rust for now. 1
means a primitive, non-composite type, which ArcStr
represents, the same way as String
and similar.
@tyranron an idea: Could we somehow make From<ArcStr>
a supertrait of ScalarValue
? Then one could make a custom scalar value that doesn't need to clone string buffers that are already ArcStr
. Think introspection queries, etc.
(Ideally From<&ArcStr>
(which doesn't require cloning up front), but that's not possible without making ScalarValue
lifetime generic. Another idea is to add a method fn from_arcstr(&ArcStr) -> Self
to trait ScalarValue
itself.)
edit: fn from_arcstr(&ArcStr)
could be a method with a default implementation that just delegates to From<String>
@tyranron I see there are some compile errors on the bikeshedded changes now. Should I help out with trying to resolve those?
Fixes #819
This is an experiment that uses arcstr::ArcStr for storing strings within the GraphQL schema. String literals that are constant/come from macros no longer allocate memory.
ArcStr
appears to fit perfectly for juniper in my opinion. It delivers extremely cheap literal strings, which is the most common usecase, while also supporting dynamic schemas based on extensive use of customTypeInfo
(which is how we're using juniper at work), potentially without extra memory allocation. There's of course the "cost" that users have to see ArcStr in the dynamic schema building APIs, but I think that with these new APIs exposed it should be much clearer when juniper needs to allocate string memory and where it's "free" (usingliteral!
). In short, this choice is now put in the hands of the user.BREAKING CHANGE:
GraphQLValue::type_name
andGraphQLType::name
both returnOption<ArcStr>
instead ofOption<&str>
.Benchmark results
master
arcstr
All of the improvements should be related to schema creation, not the query executor.