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

Refactor: Add list_select_related for performance improvement #810

Conversation

mahdirahimi1999
Copy link
Member

@mahdirahimi1999 mahdirahimi1999 commented May 25, 2024

This pull request aims to enhance the performance of the djangorestframework-simplejwt by optimizing database queries in the admin interface. By strategically using the list_select_related attribute in the admin classes OutstandingTokenAdmin and BlacklistedTokenAdmin, we significantly reduce the number of database hits required to render the admin list pages.

Changes Made:

  • OutstandingTokenAdmin:
    • Added list_select_related = ("user",) to select related user objects efficiently.
  • BlacklistedTokenAdmin:
    • Added list_select_related = ("token__user",) to select related token user objects, improving performance.

Motivation:

In the current implementation, the admin list pages execute separate database queries for each related object displayed in the list. This approach becomes inefficient when dealing with large datasets, leading to performance bottlenecks. By explicitly specifying the related fields to be included in the query results, we eliminate the need for additional database hits, resulting in a notable performance improvement.

Impact:

  • Performance Enhancement: This optimization reduces the database load and improves the responsiveness of the admin interface, especially when managing a large number of objects.
  • Maintainability: The added list_select_related attributes adhere to best practices and contribute to cleaner, more efficient code.

Testing:

All existing tests have been run to ensure that the functionality remains intact after the optimization. Additionally, manual testing has been performed to validate the improved performance in real-world scenarios.

Future Considerations:

Continued monitoring and profiling of performance metrics will be beneficial to gauge the effectiveness of this optimization over time. Further refinements may be made based on feedback and performance analysis.

@Andrew-Chen-Wang
Copy link
Member

How is this different? It does the same thing; also this would break existing users' functionality. Can you provide screenshots of the aforementioned N+1 queries?

@mahdirahimi1999
Copy link
Member Author

mahdirahimi1999 commented May 28, 2024

How is this different? It does the same thing; also this would break existing users' functionality. Can you provide screenshots of the aforementioned N+1 queries?

Thank you for your feedback and for taking the time to review the pull request.

This file change does not impact performance and optimization queries, but by using list_select_related, we adhere to the best practices recommended by Django and Django REST Framework. This not only improves code readability but also maintains consistency with the framework's conventions.

Link to relevant code in django project

@Andrew-Chen-Wang
Copy link
Member

Andrew-Chen-Wang commented May 28, 2024

The purpose of placing this in get_queryset is for users to be able to inherit the class and override necessary details. Changing it will break current users' implementations. Libraries have different goals from projects. Thanks for the PR!

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.

2 participants