Query explosion even on simple models with no included serializers

[aidanlister]

I love the idea of this module but it seems like it's impossible to build fast APIs with it.

Let's say I have Books and Authors. I fetch a list of books, even with no "included" specified I get a query for each author, just to display the FK?

As a broader issue, as Author increases in complexity developers will add more included_serializers to that serialiser, which inadvertently affects my Books viewset with a query explosion.

Could we define a lambda for each included_serializer to alter the query? Anything more robust than magic.

[mblayman]

@aidanlister thanks for your concern about performance. As a production user of DJA, I want my site to be performant too. The built-in DJA ModelViewSet uses a prefetch_related on querysets to avoid an N+1 query bug when including additional models. Do you have a specific example and/or data to explain the "query explosion" that you're referring to?

FYI, there was a recent bug that was fixed (which I don't think has been released yet) that was preventing the prefetch_related code from executing. Ideally, a DJA model viewset should only have to do 1 db query for each included model type. If that's not the behavior you're seeing in practice, then maybe there is another bug.

[aidanlister]

@mblayman so load up the example app, generate some fake data:

factories.CommentFactory.bulk_create(50)

Then look at the comments list:

We can see there's 5 comments, but 15 queries, and no included relationships ... it's doing a query just to display the FK.

This is the latest develop code.

[aidanlister]

I've added a failing test,
uptick@66609ae

[mblayman]

Thanks @aidanlister, that's really helpful info. I agree that seems wrong for sure.

It seems like the next steps are to track down those excessive queries and find a way to delay evaluation or group things together to reduce the number to where it should be.

[aidanlister]

@mblayman I don't think it's about delaying the evaluation, we don't need a query to just show the FK.

I'll add a series of performance test cases to properly test this, but have you thought about providing a way of specifying the select related / prefetch related via a lambda or method on the serializer? Forcing it to be explicit seems better than magic.

[mblayman]

Truthfully, I haven't given it a lot of thought. I stepped into a maintainer mode less than a couple of months ago. I'm not an expert in all the internal workings of the project, though I would like to understand it better.

Explicit over implicit definitely fits the Python ethos. I suspect that a reasonable default would be needed to maintain backwards compatibility. Adding a method to help control the select_related / prefetch_related aspects of the serializer sounds good.

I'm trying to fire up the example now so I can look at the queries and see where the library is being wasteful.

[mblayman]

Ok, I got the example app going and debug toolbar is showing me that each comment in the comment listing is adding two queries. This is definitely an N+1 bug because debug toolbar shows the extra queries come from fetching the author and entry from each comment to get the respective IDs. I think the problem is in the example app's viewset code.

With 4 comments, I had 12 queries. Some are unavoidable because of the need to get the count and use the paginator (as you noted in your test). 8 queries came from each of the 4 comments fetching author and entry.

I went to the CommentViewSet and changed the queryset:

    queryset = Comment.objects.all().select_related('author', 'entry')

After this change, the query count dropped to 5 and stayed there even when I added more comments.

I do see the possibility to optimize the model serializer to use entry_id instead of entry.id (for example). That would be a nice benefit to avoid going to the entry db table entirely since the ID is stored in the foreign key field of the comment model. I think this pattern could generalize.

[aidanlister]

Ahhhh, yes you're right, it's the author.id that's causing it to blow out. If we can fix that, that solves a good chunk of my issue.

[aidanlister]

Looks like line 120 in renderers.py,
resolved, relation_instance = utils.get_relation_instance(resource_instance, source, field.parent)

?

[aidanlister]

Actually I take that back ... the renderer has no effect.

Line 143, Json API ModelSerializer has this:
serializer_related_field = ResourceRelatedField

Somewhere in ResourceRelatedField ...

[aidanlister]

So, on ResourceRelatedField, we set use_pk_only_optimization = False,

class ResourceRelatedField():
    def use_pk_only_optimization(self):
        # We need the real object to determine its type...
        return False

This means that this method from the parent class (PrimaryKeyRelatedField) runs:

    def get_attribute(self, instance):
        if self.use_pk_only_optimization() and self.source_attrs:
            # Optimized case, return a mock object only containing the pk attribute.
            try:
                instance = get_attribute(instance, self.source_attrs[:-1])
                value = instance.serializable_value(self.source_attrs[-1])
                if is_simple_callable(value):
                    # Handle edge case where the relationship `source` argument
                    # points to a `get_relationship()` method on the model
                    value = value().pk
                return PKOnlyObject(pk=value)
            except AttributeError:
                pass

        # Standard case, return the object instance.
        return get_attribute(instance, self.source_attrs)

The optimized case is what stops the query explosion ordinarily, it creates a mock object (PKOnlyObject) with only the PK set so that there's no object lookup.

@jsenecal looks like you made the commit two years ago, any insight?

23d0af6

[aidanlister]

I think I'm getting there ... this has resolved the number of queries, but I had to kill some functionality.

uptick@94f79c4

[aidanlister]

I think I've gone as far as I can go with this ...

This is a simple pattern I've used for managing the queryset based on what is included, I think this makes sense:

class CommentViewSet(viewsets.ModelViewSet):
    queryset = Comment.objects.all()
    serializer_class = CommentSerializer

    def get_queryset(self):
        qs = super().get_queryset()
        if 'author' in self.request.GET.get('include', '').split(','):
            qs = qs.prefetch_related('author')
        return qs

The other location I mentioned before was also causing an explosion, the simplest fix is removing the get links functionality:

field_links = field.get_links(resource_instance)

[aidanlister]

This is where I've finished up:

https://github.com/django-json-api/django-rest-framework-json-api/compare/develop...ABASystems:hotfix/performance?expand=1

I've eliminated the query explosion for the simple case ... I've done this by switching off the links at the settings level, and by passing the type of the object around rather than expecting the full instance to be available.

Rather than disabling the links at the settings level we should be able to disable fetching them if the resource type isn't in included, but I didn't get that far.

On the more complicated example, comments?include=author, the bug manifests itself again with a query for each AuthorBio just to display the FK ...

For now, I'm giving up, I don't get the feeling that this is easily solvable.

@jerel @jsenecal any insight onto how I might fix this would be much appreciated.

[aidanlister]

@mblayman have you had a chance to take a look at this yet?