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

Add IEnumerable and IQueryable connections #121

Merged
merged 30 commits into from
Jul 1, 2022

Conversation

myty
Copy link
Contributor

@myty myty commented Nov 11, 2021

This is to address #19 and the performance implications when creating a connection on large lists. By changing to EnumerableSliceMetrics, we should be able to also utilize IQueryable to its full potential.

There are a few more things that I'd like to focus on:

  • Add an EFCore sample project to highlight its usefulness of IEnumerable and IQueryable or update the existing sample projects to do so
  • The EnumerableSliceMetrics class still needs a little more cleanup
  • Write some benchmark tests to see what the performance implications or improvement might be between EnumerableSliceMetrics and ArraySliceMetrics.

@myty
Copy link
Contributor Author

myty commented Nov 15, 2021

I ran a benchmark for the ToConnection methods on IEnumerableConnection and ArrayConnection. Using a 10,000 record list to hold the data, I ran through a few different scenarios with retrieving records from the beginning of the list and near the end of the list. For both speed and memory performance, there are significant performance improvements.

BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19044.1348 (21H2)
Intel Core i7-7660U CPU 2.50GHz (Kaby Lake), 1 CPU, 4 logical and 2 physical cores
.NET SDK=6.0.100
  [Host]     : .NET 5.0.12 (5.0.1221.52207), X64 RyuJIT
  DefaultJob : .NET 5.0.12 (5.0.1221.52207), X64 RyuJIT
Method pageSize startIndex Mean Error StdDev Median Ratio RatioSD Gen 0 Allocated
IEnumerableConnection 10 0 6.339 μs 0.0949 μs 0.0888 μs 6.332 μs 1.00 0.00 2.1439 4 KB
ArrayConnection 10 0 22.150 μs 0.4407 μs 0.3907 μs 22.239 μs 3.50 0.10 39.9780 82 KB
IEnumerableConnection 10 9500 6.604 μs 0.1107 μs 0.1035 μs 6.601 μs 1.00 0.00 2.2354 5 KB
ArrayConnection 10 9500 182.516 μs 1.9431 μs 1.8175 μs 182.496 μs 27.64 0.39 39.7949 83 KB
IEnumerableConnection 25 250 12.342 μs 0.2206 μs 0.1955 μs 12.354 μs 1.00 0.00 4.1962 9 KB
ArrayConnection 25 250 34.509 μs 0.6703 μs 0.6583 μs 34.630 μs 2.79 0.09 41.6260 87 KB
IEnumerableConnection 100 9500 41.907 μs 0.9670 μs 2.6472 μs 40.877 μs 1.00 0.00 14.0991 29 KB
ArrayConnection 100 9500 216.566 μs 2.2395 μs 1.9852 μs 216.777 μs 5.19 0.31 50.7813 106 KB

@myty myty changed the title WIP - Add IEnumerable connections Add IEnumerable connections Nov 15, 2021
@Shane32
Copy link
Member

Shane32 commented Nov 17, 2021

@myty Let me know when you are ready for a full review of this PR. I always figured that this repo was pretty pointless if it didn't handle IQueryable -- at least for me anyway. So I'm glad to see you are taking some initiative to make it more useful. 🥇

@myty
Copy link
Contributor Author

myty commented Nov 17, 2021

Let me know when you are ready for a full review of this PR. I always figured that this repo was pretty pointless if it didn't handle IQueryable -- at least for me anyway. So I'm glad to see you are taking some initiative to make it more useful. 🥇

@Shane32 I think it's probably at a good spot to go ahead with a full review.

I've been exploring how we can incorporate GraphQL and Relay into our APIs that we've been wrtiting and I really wanted IQueryable support. I started to go down the route of just adding on top within our own internal libraries and realized it could be something that would be beneficial to everyone. 😃

I have some other improvements that I'd like to contribute as well and had to keep backing off in this PR so that it wasn't massive and incorporating stuff that wasn't relevant. I've got one issue out there to improve the relay sample application, but I've got a few more.

@myty
Copy link
Contributor Author

myty commented Nov 17, 2021

@Shane32 One thing I was debating was to remove the ArraySliceMetrics all together since it really isn't needed anymore. I can do that if you think it's ok to do that at this point. Of course it's possible someone may be have a dependency at this point so maybe marking obsolete it would a better option.

@myty
Copy link
Contributor Author

myty commented Dec 31, 2021

@Shane32 Let me know if there's anything else that I need to do for this PR, otherwise I think it's good enough for review. Thanks!

@Shane32
Copy link
Member

Shane32 commented Dec 31, 2021

Thanks for reminding me. I'll try to take a look at it this weekend.

@Shane32 Shane32 mentioned this pull request Apr 3, 2022
@myty myty changed the title Add IEnumerable connections Add IEnumerable and IQueryable connections Apr 7, 2022
@myty
Copy link
Contributor Author

myty commented Apr 7, 2022

@Shane32 @sungam3r I added IQueryable to this. If nothing else needs address, I think it's good for review.

@Shane32
Copy link
Member

Shane32 commented Apr 7, 2022

Sounds good. I'll review

@Shane32
Copy link
Member

Shane32 commented May 18, 2022

@Shane32 Looks like the CodeQL build check is still failing, but I can't tell if that is expected or not.

I wouldn't worry about it

@sungam3r
Copy link
Member

sungam3r commented May 23, 2022

Yes, I would like to review that PR again. @myty I fixed CodeQL in #148, update your branch, CodeQL workflow should be green now. As I said, I do not use this repo, but this does not mean that I do not want to use it. Perhaps this is a time to begin.

@sungam3r
Copy link
Member

Also I would like to merge #157 before that to be able to track API changes.

@sungam3r
Copy link
Member

#157 was merged

@myty
Copy link
Contributor Author

myty commented May 23, 2022

Also I would like to merge #157 before that to be able to track API changes.

Sounds good. Will merge this evening.

@myty
Copy link
Contributor Author

myty commented May 24, 2022

#157 was merged

@sungam3r I've merged master in this branch.

@sungam3r
Copy link
Member

@myty Update GraphQL.Relay.approved.txt file with generated text. Just delete old file, and rename ~.received one with old name. This file tracks API changes so we can see if any breaking changes are made.

@myty
Copy link
Contributor Author

myty commented May 24, 2022

@myty Update GraphQL.Relay.approved.txt file with generated text. Just delete old file, and rename ~.received one with old name. This file tracks API changes so we can see if any breaking changes are made.

@sungam3r I updated the GraphQL.approved.txt file and realized extensions were historically in the Utilities namespace so I moved the ResolveConnecitonContextExtensions to the Utilities folder and updated the namespace

@sungam3r
Copy link
Member

So, no breaking changes, OK. Give me 2-3 days to read about relay spec and make review.

@Shane32
Copy link
Member

Shane32 commented Jun 27, 2022

@sungam3r This should be reviewed and merged before 0.7. I already reviewed it a while ago, but you had planned to review it also.

@sungam3r
Copy link
Member

I still need to dig into relay spec, alas. Let's publish 0.7 to fix #160 . We can publish 0.8 at any time.

@codecov-commenter
Copy link

Codecov Report

Merging #121 (1d8f03b) into master (5ad6b1c) will decrease coverage by 0.69%.
The diff coverage is 61.25%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   71.96%   71.26%   -0.70%     
==========================================
  Files          14       16       +2     
  Lines         371      449      +78     
  Branches       39       43       +4     
==========================================
+ Hits          267      320      +53     
- Misses         96      123      +27     
+ Partials        8        6       -2     
Impacted Files Coverage Δ
...ay/Utilities/ResolveConnectionContextExtensions.cs 27.50% <27.50%> (ø)
src/GraphQL.Relay/Types/SliceMetrics.cs 94.87% <94.87%> (ø)
src/GraphQL.Relay/Utilities/RelayPagination.cs 100.00% <100.00%> (ø)
src/GraphQL.Relay/Utilities/StringExtensions.cs 100.00% <0.00%> (+20.00%) ⬆️
...rc/GraphQL.Relay/Utilities/EnumerableExtensions.cs 100.00% <0.00%> (+23.52%) ⬆️

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 232752b...1d8f03b. Read the comment docs.

@Shane32
Copy link
Member

Shane32 commented Jun 29, 2022

@sungam3r I already reviewed this. It's been sitting here, approved, for over a month. I'll merge it unless you plan to review this promptly.

@sungam3r
Copy link
Member

I made a quick review.

@myty myty requested a review from sungam3r July 1, 2022 14:34
myty and others added 2 commits July 1, 2022 10:35
Co-authored-by: Shane Krueger <shane@acdmail.com>
Co-authored-by: Shane Krueger <shane@acdmail.com>
@Shane32 Shane32 merged commit d7ca6f4 into graphql-dotnet:master Jul 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants