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

ISSUE-457 - Improve performance for I/O bound list resolvers by using parallel execution #458

Merged
merged 2 commits into from Dec 7, 2020

Conversation

Maximilien-R
Copy link
Member

Closes #457.

@abusi
Copy link
Contributor

abusi commented Nov 12, 2020

Seems really cool @Maximilien-R

@Maximilien-R
Copy link
Member Author

Maximilien-R commented Nov 12, 2020

Hi @abusi & @garyd203,

Following our discussions on the #457 issue, here is a first draft aiming to fix it.

Currently, I do fix only the list output coercion aspect. On the issue thread, I suggested to do something similar for output object coercion however this topic will be less straight forward. Indeed, there is the case for abstract object (Union & Interface) that should be discuss before going forward. Also, since on tartiflette we do build all the coercer function at engine baking its harder to manipulate the object/abstract coercion.

I think we could go on the object/abstract coercion customization on another PR. What I would like to do it to be able to really fine tuning it per object field instead of the parent field like its done for list.

For instance, if we had the following SDL:

type Author {
  id: ID!
  firstName: String!
  lastName: String!
  birthdate: Date!
}

type Book {
  id: ID!
  title: String!
  authors: [Author!]
  nbPages: Int!
  resume: String!
  opinions: [String!]
}

type Query {
  books: [Book!]
}

We could imagine that the Query.books resolver will be able to return most of the data necessary to resolve each Book but we could need for the Book.authors & Book.opinions fields to have a dedicated resolver in order to fetch the data on another data source.

As it's designed currently, we'll asyncio.gather all the Book fields either it call the default resolver or a dedicated one. What I could like to do (test) is that we could define which field should be asyncio.gather (those which do I/O) and which field should be coerce sequentially.

We could have something like:

@Resolver("Query.books")
async def resolve_query_books(parent, args, ctx, info):
  return Books.objects.all()


@Resolver("Book.authors", concurrently=True)
async def resolve_book_authors(parent, args, ctx, info):
  return Authors.objects.filter(pk__in=parent["authors"]).all()


@Resolver("Book.opinions", concurrently=True)
async def resolve_book_authors(parent, args, ctx, info):
  return Opinions.objects.filter(book_id=parent["id"]).all()

The aim is to avoid to create async task for resolver that will only fetch data through a dumb (or the default) resolver which will only do a getattr or getitem and asyncio.gather only field which do I/O tasks and that performance would be improved by running them concurrently. Of course it'll be a feature which will be used only when you want a full control on your resolve process and when you need to have extra performance.

The difference with the list is that its the field resolver which will impact the parent resolving process.

Tell me if I'm not clear 😆

@abusi
Copy link
Contributor

abusi commented Nov 12, 2020

I agree that it should be discussed in another PR, we'll keep this one for List only so it's clearer :)

@garyd203
Copy link
Contributor

Looks good @Maximilien-R thank you very much

I understand what you are saying about only doing concurrent resolution if there is a field that needs I/O (ie. the field has a non-dumb resolver), and agree. I suspect that applies to the list case too, not just objects - a list only really needs a concurrent resolver because one or more of the fields on each item needs concurrent resolution. So it seems broadly similar, just 1 step further removed (list_field -> item_in_list_field -> field_on_item_with_non_default_resolver)

@abusi
Copy link
Contributor

abusi commented Dec 6, 2020

@Maximilien-R shall we merge ?

@Maximilien-R
Copy link
Member Author

Maximilien-R commented Dec 6, 2020

@abusi, I guess we could if it's fine for you 👍

Note: if we want to do something similar with objects (according to my previous comment) we should probably change the parameter name to focus on the list aspect of concurrently parameter.

@sonarcloud
Copy link

sonarcloud bot commented Dec 6, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
1.5% 1.5% Duplication

@abusi abusi merged commit 397feb5 into master Dec 7, 2020
@abusi abusi deleted the ISSUE-457 branch December 7, 2020 07:22
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.

Improve performance for I/O bound list resolvers by using parallel execution
3 participants