-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Lazily fetch album in FormattedItemMapping, it's not needed in most cases #3260
Lazily fetch album in FormattedItemMapping, it's not needed in most cases #3260
Conversation
So far so good! Maybe we could make the code a little more legible by introducing a simple def lazy_property(func):
field_name = '_' + func.__name__
@property
@functools.wraps(func)
def wrapper(self):
if hasattr(self, field_name):
return getattr(self, field_name)
value = func(self)
setattr(self, field_name, value)
return value
return wrapper This would work for all three of the properties that are currently manually made lazy, if I'm not mistaken. |
Neat! I pushed a new commit, introducing |
I looked into adding some tests, but it seems like |
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.
Nice!! This looks great. And I agree that the test coverage is probably already good here. Would you mind doing just a few menial tasks?
- Let’s move the new decorator to
util
. It could be useful elsewhere. - A short docstring for the decorator would be nice too.
- Maybe a quick changelog entry would be in order.
Then we can measure the performance impact of this in isolation (both with and without album-level fields) and merge it!
Sure, I put
Not a very thorough benchmark, but it looks pretty significant. Queries that don't use album level fields run about 1.5x slower on |
Nice! I ran my own little performance test here, with three runs for each of the 4 treatments:
Long story short, that's a 1.7x speedup for the default format and no significant change for the format that includes an album-level field. Wahoo!!! |
Merged! Just for fun, I did a quick comparison between the new |
Broken out from #3258, as it seems orthogonal.
The item formatter is fetching the related album for each item, even when it is not needed. This PR lazily fetches the album if it is really needed, and does not hit the DB in other cases.