Skip to content
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

Caching #53

Merged
merged 19 commits into from
Jan 19, 2020
Merged

Caching #53

merged 19 commits into from
Jan 19, 2020

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Dec 19, 2019

It replaces #50

I continued my experiments and came to the conclusion that performance can be improved by caching strings. It is some sort of "smart local string interning" without actually string allocation. Allocations can be minimized. On the other hand, this requires additional computational load due to the calculation of hashes and comparisons. Nevertheless, there is a tendency towards a total decrease in computational load. I made two cache implementations. Now it’s clear that an implementation with pointers is practically no better than a managed implementation (quite possibly, I wrote it incorrectly). It is given more likely as an example and in the future will most likely be removed from the PR. Another cache implementation for a multi-threaded scenario will also appear. I expect small steps to make changes, gradually increasing performance.

Progress can be seen here: https://github.com/sungam3r/parser/blob/caching/src/GraphQLParser.Benchmarks/GraphQLParser.Benchmarks.Reference.md
Without a doubt, there is still room for improvement.

[UPDATE]: Unsafe code was removed.

Current perf summary report:


Baseline (B) to current (C) comparison without cache:

| Method |  query |  Mean (B) |  Mean (C) | Ratio | Allocated (B) | Allocated (C) | Ratio |
|------- |------- |----------:|----------:|------:|--------------:|--------------:|------:|
|  Parse | Params | 13.911 us | 10.291 us |  0.74 |      18.85 KB |       8.59 KB |  0.46 |
|  Parse | Schema | 30.629 us | 23.741 us |  0.77 |      36.49 KB |      19.09 KB |  0.52 |
|  Parse | Simple |  2.391 us |  1.757 us |  0.73 |       3.58 KB |       1.63 KB |  0.46 |

Baseline (B) to current (C) comparison with cache:

| Method |  query |  Mean (B) |  Mean (C) | Ratio | Allocated (B) | Allocated (C) | Ratio |
|------- |------- |----------:|----------:|------:|--------------:|--------------:|------:|
|  Parse | Params | 13.911 us | 10.895 us |  0.77 |      18.85 KB |       7.82 KB |  0.41 |
|  Parse | Schema | 30.629 us | 26.577 us |  0.85 |      36.49 KB |      15.66 KB |  0.43 |
|  Parse | Simple |  2.391 us |  1.884 us |  0.78 |       3.58 KB |       1.35 KB |  0.38 |

Baseline (B) to current (C) comparison with concurrent cache:

| Method |  query |  Mean (B) |  Mean (C) | Ratio | Allocated (B) | Allocated (C) | Ratio |
|------- |------- |----------:|----------:|------:|--------------:|--------------:|------:|
|  Parse | Params | 13.911 us | 11.113 us |  0.80 |      18.85 KB |       7.82 KB |  0.41 |
|  Parse | Schema | 30.629 us | 26.607 us |  0.87 |      36.49 KB |      15.66 KB |  0.43 |
|  Parse | Simple |  2.391 us |  1.989 us |  0.83 |       3.58 KB |       1.35 KB |  0.38 |

@sungam3r sungam3r added the enhancement New feature or request label Dec 19, 2019
@sungam3r
Copy link
Member Author

@joemcbride What do you think about making LexerContext and ParserContext internal? These classes do not actually participate in interactions with public api.

@sungam3r sungam3r changed the title [WIP] Caching Caching Jan 4, 2020
@sungam3r
Copy link
Member Author

sungam3r commented Jan 4, 2020

@joemcbride PR is ready for review. The memory traffic gain is about 2 times and the execution time is 15-25 percent less. I bumped the major version of the parser to 5.0.0 as part of the classes became structures.

@sungam3r
Copy link
Member Author

@joemcbride If you don't mind I would like to merge this before working on #56 and #57 .

@sungam3r
Copy link
Member Author

So we can assume that there is no objection so I merge this PR to use the new parser in graphql-dotnet repo.

@sungam3r sungam3r merged commit a3ddd38 into graphql-dotnet:master Jan 19, 2020
@sungam3r sungam3r deleted the caching branch January 19, 2020 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant