Merge ~pappacena/launchpad:gitrepo-pending-merges-tuning into launchpad:master
| Status: | Needs review |
|---|---|
| Proposed branch: | ~pappacena/launchpad:gitrepo-pending-merges-tuning |
| Merge into: | launchpad:master |
| Diff against target: |
363 lines (+191/-28) 4 files modified
lib/lp/code/browser/branchmergeproposal.py (+64/-18) lib/lp/code/browser/gitrepository.py (+64/-5) lib/lp/code/browser/tests/test_gitrepository.py (+62/-4) lib/lp/code/templates/branchmergeproposal-summary-fragment.pt (+1/-1) |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Launchpad code reviewers | Pending | ||
|
Review via email:
|
|||
Commit message
Small improvements in performance and allowing to limit the amount of MPs displayed on repository-
Description of the change
This is not exactly ready to be merged, but I would like some opinions.
I've checked with @profilehooks.
There are two places spending quite a lot of processing time:
1- Fetching and counting MPs to show the /+activereivews link
This part fetches all MPs (and they can be a lot) just to count. It cannot be done on the database directly because we need to use Zope to check for permissions. I'm proposing to limit this count to a certain number, and after that just show "more than <limit> MPs" to the user.
2- List the summary of the most recent MPs for each branch
Again, this cannot be totally filtered on the database because of Zope's permission check. But, apart from limiting, I'm proposing some small improvements on the method getting the latest MP per branch.
On the test side, while I was coding the changes, I've wrote a small test to check the processing time. I'm including a slightly different version of what I've writen before, but I'm not sure if we should include this type of check based on the processing time. Specially because this is dependent on the machine running and it might eventually break under unexpected situations (think of a busy buildbot, for example).
Anyway, I'll probably need to polish a bit this MP, but I would like to have some early feedback on it.
| Colin Watson (cjwatson) wrote | # |
There was an error fetching revisions from git servers. Please try again in a few minutes. If the problem persists, contact Launchpad support.
I think it should be possible to express an equivalent of the launchpad.View permission check on merge proposals directly in a database query. We have a good chunk of the machinery for that already (BranchCollection and GitCollection), and I think we just need to use it more effectively here.
As for the remaining limit stuff, it looks to me as though this needs to be reworked using the batch navigation system; that's normally how we handle rendering large collections.
We normally do avoid wallclock speed tests for the sorts of reasons you suggest. In most cases query count tests (especially tests for how query counts scale with the number of objects involved) are good enough proxies, and they have the great virtue of behaving deterministically. I think that would be the case here.
There's a lot of stuff to get your teeth into here! Let's discuss it at the sprint, since that's so close.