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

Benchmarking and Performance Improvements #5614

Closed
carltongibson opened this issue Nov 21, 2017 · 10 comments
Closed

Benchmarking and Performance Improvements #5614

carltongibson opened this issue Nov 21, 2017 · 10 comments

Comments

@carltongibson
Copy link
Collaborator

carltongibson commented Nov 21, 2017

Over the years we've had a number of suggestions (collected below) for improving DRF performance. These have often stalled for lack of a suitable benchmarking measure.

Progress on these issues requires:

  • A benchmarking project that we can keep up to date and run repeatedly against proposed changes (and the base branch for comparison).

With that in place we could sensibly assess changes, and either accept or reject them, rather than leaving them to rot indefinitely in the issue tracker.

Design Note: Performance isn't a primary goal of Django REST Framework. Rather we're into clarity and ease-of-use. (Other speed-ups, e.g. caching at another level, are often the way to go anyway.) Clean changes that are performance wins are still wins: they're worth having.

Related Tickets:

List of tickets suggesting improvements. Not necessarily complete (— Do add relevant issues!). Blocked by lack of benchmarking. Worth revisiting once we have a solution in place.

@xordoquy
Copy link
Collaborator

I have a couple handy that I need to benchmark as well.

@carltongibson
Copy link
Collaborator Author

carltongibson commented Nov 21, 2017

@xordoquy Please add them to the list! 🙂

I'm closing the related tickets as blocked until we can solve this benchmarking issue: then we'll (be able to) re-open/re-assess, and make a final decision.

@JockeTF
Copy link

JockeTF commented Nov 21, 2017

I nominate #3007.

@carltongibson
Copy link
Collaborator Author

carltongibson commented Nov 22, 2017

@JockeTF Yes. Good call. Thanks. That ticket also links to @thedrow's https://github.com/thedrow/drf-benchmarks project, which I had forgotten about. Maybe there's ideas was can pick up from there.

@tomchristie
Copy link
Member

I'd like to close this off. Perfectly happy to see benchmarking tools, but I don't think there's any obvious need to address this as part of the core project. Number-of-query issues are already handled in the issue tracker, and in any case are now all pretty matured. Serializers have a bit of overhead for very large unpaginated list responses, but the large number of API hooks prevents us being able to make much in the way of changes there. I'd expect the only substantial improvements on that side would need to be an alternative fresh serializer implementation. Anyways short version - I don't think we need an actively open ticket for creating a benchmark right now.

@arski
Copy link

arski commented Jan 3, 2019

Hi there, are you sure about this? we have overridden the default get_fields with the following and on some requests which serialize 500 objects with an average of 2-3 nested in each we see an improvement from 60s per response to 20s... There can be arguments made whether such an API endpoint is well designed, but the bottom line is that the caching most definitely improves our APIs performance across the site, even for small endpoints...

class CachedModelSerializer:
    _cached_get_fields_response = {}

    def get_fields(self):
        if self.__class__ not in self.__class__._cached_get_fields_response:
            self.__class__._cached_get_fields_response[self.__class__] = super().get_fields()

        return copy.deepcopy(self.__class__._cached_get_fields_response[self.__class__])

PS I realize the solution is an exact duplicate of #2504 (comment)

@davidszotten
Copy link
Contributor

@carltongibson sorry for posting on an old issue, but i was wondering if you could link to a pr/ticket that's the source for you crossing out "Calling reverse is very expensive" as (Fixed in Django)

Thanks!

@rpkilby
Copy link
Member

rpkilby commented Nov 1, 2019

HI @davidszotten, see #3007 (comment).

@davidszotten
Copy link
Contributor

HI @davidszotten, see #3007 (comment).

Thanks for the quick response! Maybe i'm missing something but isn't that about build_absolute_uri (as opposed to reverse)?

@rpkilby
Copy link
Member

rpkilby commented Nov 1, 2019

I guess the two are often used in conjunction with each other (e.g., when serializing hyperlinked models), so speeding up build_absolute_uri generally speeds up URL resolution. You're right that it doesn't look like the performance of reverse was improved.

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

No branches or pull requests

7 participants