-
Notifications
You must be signed in to change notification settings - Fork 264
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 caching of OptInFieldsMixin fields property #1093
Conversation
FYI in my testing with 5k devices, this improves response time ~2x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment is minor. Just thinking perhaps a chance to cleanup something that doesn't appear to need to be there, but otherwise this is good to go!
Does it make sense to patch to the 1.1 version? |
Yes please, I came here to ask the same question. |
I will add a +1 on the patch of 1.1.x |
I'm working on preparing a 1.1.6 release to include this fix |
* Add caching of opt-in fields pruned_fields property * Address review comment
Fixes: #N/A
Profiling of API requests with Silk showed that one potential bottleneck was repeated calls to
serializer.fields
, which is an instance attribute in baseline django-rest-framework but is overridden into a property in ourOptInFieldsMixin
class. It appears that this property was accessed about 9 times per model instance being retrieved. As a result, adding some basic caching here pays performance dividends - in my local testing, using the development server running in Docker, an API query to retrieve 1000 Devices (minus config-context) is reduced from about 40 seconds to about 30 seconds.The diffs are a bit difficult to read but basically all this is, is moving most of the body of the property function into an
if self.__pruned_fields is None:
block.