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

[#2540] Allow listing more projects in the directory #2541

Merged
merged 6 commits into from
Mar 8, 2017

Conversation

punchagan
Copy link
Contributor

The query for partnership_more_link is very slow, and adding a
select_related makes it quite a lot faster. This looks like the only query
that is slow on the project listing page.

We add a limit parameter to specify the number of projects to be listed in
the directory, with the default value being 10.

Closes #2540

  • Test plan | Unit test | Integration test
  • Copyright header
  • Code formatting
  • Documentation
  • Change log entry

Copy link
Contributor

@zzgvh zzgvh left a comment

Choose a reason for hiding this comment

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

Looks good. A pair of optional small fixes!

return True
else:
return False
budget_items = self.budget_items.all()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can do something like this:

other_currencies = self.budget_items.values_list('currency', flat=True).distinct()

to get a list of extra currencies. Might be a little quicker, letting the DB do the filtering...

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's a trade off between making an extra query vs. using the budget_items which were already pre-fetched. We probably need to measure which one is better for the average case. I'll play around a little, with this code, and get back.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the Python iteration is about a hundred times faster, when the budget_items have been pre-fetched and there are only a couple of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right and we're getting them anyway. Interesting!

from django.test import TestCase


@skip('Needs Django >= 1.8')
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool! We need to upgrade!

@@ -96,7 +96,8 @@ def directory(request):

# Build page
page = request.GET.get('page')
page, paginator, page_range = pagination(page, sorted_projects, 10)
limit = request.GET.get('limit', 10)
Copy link
Contributor

@zzgvh zzgvh Feb 22, 2017

Choose a reason for hiding this comment

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

Maybe have the default value come from settings? Gives us another way of controlling this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

The query for `partnership_more_link` is very slow, and adding a
`select_related` makes it quite a lot faster.  This looks like the only query
that is slow on the project listing page.

We add a `limit` parameter to specify the number of projects to be listed in
the directory, with the default value being 10.

Closes #2540
And optimize check for multiple currencies in a project
And refactor codelist_name function to use this.  These functions are called in
so many places, that caching them should bring significant benefits all over
the site.

RSR seems to already have memcached configured and it's a shame that we don't
use it anywhere except for thumbnails.  A lot of things can benefit from caching!
Added a simple performance test example for the project directory listing, but
the test cannot be run with the current version of Django.  It can be
un-skipped when Django is updated!
@zzgvh zzgvh merged commit 0dd0f9d into develop Mar 8, 2017
@zzgvh zzgvh deleted the #2540-more-projects branch March 8, 2017 14:43
@zzgvh zzgvh removed the Needs Review label Mar 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants