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

Optimize location conversion #202

Merged
merged 3 commits into from
Jan 22, 2022
Merged

Optimize location conversion #202

merged 3 commits into from
Jan 22, 2022

Conversation

sungam3r
Copy link
Member

@sungam3r sungam3r commented Jan 21, 2022

I spent on writing and debugging this algorithm a bunch of time. Although this code is performed only when making exception messages, I wanted to get rid of the regular expression. No memory allocations at all now. If you check code before changes on a rather big query, then you will make sure that the work with a regular expression here generates an abnormal amount of memory allocations.

fixes #102

@sungam3r sungam3r added this to the 8.0 milestone Jan 21, 2022
@sungam3r sungam3r requested a review from Shane32 January 21, 2022 23:53
@sungam3r sungam3r self-assigned this Jan 21, 2022
@github-actions github-actions bot added the test Pull request that adds new or changes existing tests label Jan 21, 2022
/// <br/>
/// <br/><br/>
/// This struct is designed to unify work with objects of <see cref="ReadOnlyMemory{T}"/>,
/// <see cref="ReadOnlySpan{T}"/> ans <see cref="string"/>. For example, you can directly
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shane32 I added these notes to make ROM usage by other projects more understandable.

@sungam3r
Copy link
Member Author

sungam3r commented Jan 22, 2022

Example:

        string d = "KitchenSink".ReadGraphQLFile();

        for (int i = 0; i < 100; ++i)
        {
            var m1 = GC.GetAllocatedBytesForCurrentThread();
            var loc = new Location(d, 3000);
            var m2 = GC.GetAllocatedBytesForCurrentThread();
            var delta = m2 - m1;
        }

var matches = _lineRegex.Matches((string)source); returns collection with 152 items. Each loop iteration increases memory by 35968 bytes! After changes delta=0 on each loop iteration.

@sungam3r
Copy link
Member Author

I'll add tests for 100% coverage later, 3:30AM here.

@sungam3r
Copy link
Member Author

The desire to get full coverage again made it possible to find a bug.

@codecov-commenter
Copy link

Codecov Report

Merging #202 (82772f2) into master (16376af) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #202   +/-   ##
=======================================
  Coverage   98.75%   98.76%           
=======================================
  Files          85       85           
  Lines        4411     4438   +27     
  Branches      434      437    +3     
=======================================
+ Hits         4356     4383   +27     
  Misses         33       33           
  Partials       22       22           
Impacted Files Coverage Δ
src/GraphQLParser/ROM.cs 100.00% <ø> (ø)
src/GraphQLParser/Location.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16376af...82772f2. Read the comment docs.

@sungam3r sungam3r merged commit 9316454 into master Jan 22, 2022
@sungam3r sungam3r deleted the no-regex branch January 22, 2022 10:02
@sungam3r sungam3r linked an issue Jan 22, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance test Pull request that adds new or changes existing tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eliminate RegExp usage and ToString call in Location.cs
3 participants