-
Notifications
You must be signed in to change notification settings - Fork 42
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
string -> ROM #101
string -> ROM #101
Conversation
# Conflicts: # src/GraphQLParser.Benchmarks/Benchmarks/LexerBenchmark.cs # src/GraphQLParser.Benchmarks/Benchmarks/ParserBenchmark.cs
It should also be noted that this PR opens the way for further optimizations at the GraphQL.NET level. We can now reuse the same portions of the pooled memory over and over. Of course this will require changes to GraphQL.NET because it manipulates strings from the parser. Now there are no strings, only ROM. In principle, it is feasible to achieve not spawning strings at all. Public API hasn't changed much. After that, we can evaluate the final performance |
develop
rom
|
Thanks. I updated results in my initial comment after making Lexer and Parser static. |
Ditto |
I looked through the code once. Is it ready to be merged or are you still working on it? I would like to take another look. Also, I think we should not publish this to nuget until 4.0 is ready to be released, as we may find more changes we want to make. We can reference the development build via nuget from GraphQL.NET |
Co-authored-by: Shane Krueger <shane@acdmail.com>
Co-authored-by: Shane Krueger <shane@acdmail.com>
Finally! I was able to achieve a noticeable improvement by throw helpers to allow code inlining. I updated results in my initial comment. |
Also interesting dotnet/runtime#13347 |
# Conflicts: # src/GraphQLParser.ApiTests/ApiApprovalTests.cs # src/GraphQLParser.ApiTests/GraphQL-Parser.approved.txt # src/GraphQLParser/AST/GraphQLDocument.cs # src/GraphQLParser/LexerContext.cs # src/GraphQLParser/Parser.cs # src/GraphQLParser/ParserContext.cs
# Conflicts: # src/GraphQLParser.ApiTests/GraphQL-Parser.approved.txt # src/GraphQLParser/Parser.cs # src/GraphQLParser/ParserContext.cs
private List<GraphQLNamedType>? ParseImplementsInterfaces() | ||
{ | ||
List<GraphQLNamedType>? types = null; | ||
if (_currentToken.Value?.Equals("implements") == true) | ||
if (_currentToken.Value == "implements") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shane32 With new syntax sugar all conditions look better.
} | ||
|
||
/// <inheritdoc/> | ||
public override int GetHashCode() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken from string
implementation.
Co-authored-by: Shane Krueger <shane@acdmail.com>
This PR brings
System.Memory
bits into lexer and parser. Also redesigned to reduce the number of memory allocations. Lexer has stopped allocating memory on the heap at all (with rare exceptions for escaped symbols handling). The parser still allocates memory, since it builds AST, but already noticeably less. Benchmarks, however, do not show the unconditional leadership of the new version in terms of execution time. I have noticed that my results vary from run to run.Lexer before/after
Parser before/after
@Shane32 Please try run
LexerBenchmark
andParserBenchmark
.