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

perf: relegate optgroup calculation to superclass #4540

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zawan-ila
Copy link
Contributor

PROD-4274

Description

4538 seems to have improved the perf by about 45% (on stage) which is lesser than what I expected.

Refactor SortedModelSelect2Multiple.optgroups to call the superclass optgroups to retrieve the options and then sort them accordingly. This is an attempt at further improving the performance. A side effect is that it simplifies the code.

Copy link
Contributor

@DawoudSheraz DawoudSheraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we gaining anything from perf point of view by calling super optrgoups only (I am assuming the code is same)? Or is it only achieving refactoring goals?

@zawan-ila
Copy link
Contributor Author

zawan-ila commented Jan 10, 2025

The super code is somewhat different. It does some mangling around of self.choices which allows it to process "less" choices. The idea is the same as 4538 i.e lesser choice processing = better perf. The refactoring is an added bonus.

Edit: In particular, the superclass restricts the self.choices.queryset to only the selected choices. Hence, preventing needless processing of unselected choices.

def test_optgroups_are_sorted(self, value, result_order):
choices = ((1, 'one'), (2, 'two'), (3, 'three'))
widget = SortedModelSelect2Multiple(url='requiredurl', choices=choices)
result = widget.optgroups('test', value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to do a simple optgroups test like this now since the method calls the super method, which expects things to be set up much more properly. e.g requiring self.choices to have a queryset attribute, having the url kwarg of the widget being an actual reversible url etc.

I have added a new test in place, which renders the program admin form, and ensures that the courses are rendered in the order in which they were assigned. I am unsure if this test is more appropriate in test_admin.py now.

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.

3 participants