Skip to content

Conversation

@alexxxxey
Copy link

Added simple caching for lookup_field() in UnfoldAdminReadonlyField.
Without it, custom computed fields in inlines are re-evaluated multiple times per render, causing performance issues or side effects.

@lukasvinclav
Copy link
Contributor

Thanks for PR. I'm just wiring you that I will take a look at this.

@lukasvinclav
Copy link
Contributor

Can you provide more information about the side effects or performance issues? I'm already checking the implementation and I'm trying to find edge cases which I have to test. For example if you have an issue with duplicated SQL queries, provide instructions how to can I replicate because on the demo I'm not able to find anything.

@lukasvinclav
Copy link
Contributor

btw I drafted another PR which will probably fix your issue: #1651

@alexxxxey
Copy link
Author

For example, i have this inline

class AdRawFieldInline(TabularInline):
    model = AdRawField
    extra = 0
    fields = ['key_cleaned', 'value', 'value_cleaned', 'get_correlation_human']
    readonly_fields = ['get_correlation_human', 'key_cleaned', 'value_cleaned']
    def get_correlation_human(self, obj):
        res = []
        ans = ObjectType.objects.all()
        print('>>>>>>> DB HIT <<<<<<<<')
        return ans

I add prints in

class AdminReadonlyField:
    def __init__(self, form, field, is_first, model_admin=None):
        # Make self.field look a little bit like a field. This means that
        # {{ field.name }} must be a useful class name to identify the field.
        # For convenience, store other field-related data here too.
        print(f'=========== {field} ===============================')
...

and in each is_json, is_image etc.. Also add prints

...
        print('is_json', field, obj,)
        try:
            f, attr, value = lookup_field(field, obj, model_admin)
        except (AttributeError, ValueError, ObjectDoesNotExist):
            return False
....

And in console output:

=========== get_correlation_human ===============================
is_json get_correlation_human attributes: Windows - Tinted rear
>>>>>>> DB HIT <<<<<<<<
is_image get_correlation_human attributes: Windows - Tinted rear
>>>>>>> DB HIT <<<<<<<<
contents get_correlation_human attributes: Windows - Tinted rear
>>>>>>> DB HIT <<<<<<<<
is_file get_correlation_human attributes: Windows - Tinted rear
>>>>>>> DB HIT <<<<<<<<
contents get_correlation_human attributes: Windows - Tinted rear
>>>>>>> DB HIT <<<<<<<<
=========== url ===============================
....

I noticed that lookup_field is called multiple times for the same readonly field (e.g. get_correlation_human), which triggers repeated DB hits within one inline rendering cycle.
In my PR I added a minimal caching mechanism around lookup_field to prevent redundant calls.

I intentionally kept the change very localized — without refactoring the overall logic — just to highlight the root cause and avoid breaking anything unintentionally. :))

Your proposed PR looks cleaner and more correct architecturally, but I haven’t had a chance to test it yet.

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