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

feat(django_getter): use function cache to increase performance #1150

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yorickr
Copy link

@yorickr yorickr commented May 3, 2024

Hi!

After using ninja a lot, I noticed some performance benefit could be made in DjangoGetter, by caching the actual function being called and using a lookup, instead of doing up to 4 checks each time.

This MR implements this simple cache.
I didn't specifically add tests for this, as schema is tested very well throughout the rest of the tests. Let me know if you'd like to see this differently.

Let me know what you think!

Some data:

Old code: Average time across 20 runs of 100k nested objects is 5.516484143049911
New code: Average time across 20 runs of 100k nested objects is 4.045871758400063
This is 1.5s saved, or 27% saved.

Old code: Average time across 20 runs of 10k nested objects is 0.4554609017498933
New code: Average time across 20 runs of 10k nested objects is 0.35757346044997573
This saved 22%.

@yorickr yorickr force-pushed the django-getter-performance branch 4 times, most recently from 67ac1be to 0bee043 Compare August 2, 2024 12:44
@yorickr
Copy link
Author

yorickr commented Sep 16, 2024

Hi! Is this something you would like to see included? Let me know

@yorickr
Copy link
Author

yorickr commented Oct 23, 2024

@vitalik is there any interest in merging or introducing this? If not, I can close it. Please let me know :)

@vitalik
Copy link
Owner

vitalik commented Oct 24, 2024

Hi @yorickr
Thank you for PR - I am looking into this (just afraid can be some edge cases with cache key)
On the other hand there might be a chance that pydantic will give us a nicer hooks (as DjangoGetter is actually a bit dirty workaround) and that may speed things up even more

@yorickr
Copy link
Author

yorickr commented Oct 28, 2024

Thanks for your reply :)
From what I can tell, the cache key should work fine for everything. Perhaps not for builtins, as they do not have a name. But I am unsure whether they can even end up being fed in through the constructor though.
For what it's worth, this has seen some production use and hasn't been giving us issues yet.

I agree with DjangoGetter being a bit of a dirty workaround. Until we get nicer hooks, I think this is a decent approach.

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