[shipping] Simplify and optimize teamsnippets queries

RESOLVED FIXED in 2.1

Status

P1
normal
RESOLVED FIXED
7 years ago
2 years ago

People

(Reporter: peterbe, Assigned: Pike)

Tracking

Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
This is a follow up from landing bug696446.

For every tree, to fetches the ``run.suggested_shortrev`` which is either a boolean or a changeset revision string. The call looks like this::

            __, __, run.suggested_shortrev = \
              annotated_pushes(repo, run.appversion, loc, actions)

On average, that call requires 6.2 SQL queries and for a decent sized locale, there are about 10-11 trees for each locale. Each call takes on average 0.03 seconds which isn't much but totals on average 0.34 seconds per locale (e.g. /teams/nb-NO) which is quite a lot. 

Considering that the annotated_pushes() function does a LOT of other stuff, try to figure out a way to get the suggested_shortrev in a simpler way. 
If not, consider blackboxing it with a wrapper function that uses the caching framework to store the result and have signals invalidate when any of the relevant underlying data changes.
(Reporter)

Comment 1

7 years ago
Spent some time in this bug to see if there were some quick wins. Not really :)

Commenting out the annotated_pushes() call and running debug_toolbar takes us from (on average) 175 queries to 85 queries. 

The function annotated_pushes() is a beast and not to be taken lightly. It would be great to get some help. It would be a good start to write tests for it to begin with so when we replace one black box for another we can expect the same results.
(Reporter)

Updated

7 years ago
Priority: -- → P3
(Assignee)

Comment 2

7 years ago
I'm working on this. Also setting this to a P1, as the current load time of the team page blocks going to production.

One thing that I'll do here is to change the interpretation of "suggested sign-off":

Instead of suggesting any green run, I only look at the latest. That happens to be more like it in practice. I'm also discussing this with the community in http://groups.google.com/group/mozilla.dev.l10n/browse_frm/thread/6beb0195522baa8a#.
Assignee: nobody → l10n
OS: Mac OS X → All
Priority: P3 → P1
Hardware: x86 → All
(Assignee)

Comment 3

7 years ago
Created attachment 598226 [details] [diff] [review]
honestly, not cheating.

Requesting review. Honestly, the patch looks like I'm cheating, but I'm not. I'm just using the opportunity to change my mind :-)

I've also published the feature branch at https://github.com/Pike/elmo/compare/develop...feature%2Fbug-713878-optimize-team-queries, and I'd actually like to land the three as separate changes, as they make sense each for itself.
Attachment #598226 - Flags: review?(peterbe)
(Reporter)

Comment 4

7 years ago
Comment on attachment 598226 [details] [diff] [review]
honestly, not cheating.

Review of attachment 598226 [details] [diff] [review]:
-----------------------------------------------------------------

This is perhaps weird to do but I'm going to r- because there's is still a large chunk possible to optimize. 

I ran a bunch of debug toolbar counts on the queries and for de/fr/fi/ru the number of queries went from 155/152/158/150 to 50/51/51/51 which is awesomely fantastic!

However, looking into the queries that are made, inside those blocks of of (on average) 51 queries this one is repeated at least 11 times.
SELECT ••• FROM `life_locale` WHERE `life_locale`.`id` = <XX>

Also, I noticed that there are numerous places where you get things like this:
SELECT ••• FROM `shipping_appversion` LEFT OUTER JOIN `life_tree` ON (`shipping_appversion`.`tree_id` = `life_tree`.`id`) WHERE `shipping_appversion`.`id` = 34
SELECT ••• FROM `shipping_appversion` LEFT OUTER JOIN `life_tree` ON (`shipping_appversion`.`tree_id` = `life_tree`.`id`) WHERE `shipping_appversion`.`id` = 5
SELECT ••• FROM `shipping_appversion` LEFT OUTER JOIN `life_tree` ON (`shipping_appversion`.`tree_id` = `life_tree`.`id`) WHERE `shipping_appversion`.`id` = 32

Let's see if we can instead reduce that down to 1 query which we stuff into a dict and have that available for the oncoming for loops.

I've got your branch so I'm going poke around too.
Attachment #598226 - Flags: review?(peterbe) → review-
(Assignee)

Updated

7 years ago
Blocks: 723311
(Assignee)

Comment 5

7 years ago
Created attachment 598898 [details] [diff] [review]
incorporate tweaked merge request

The pull request helped a good deal, but ended up breaking the tests, so I stripped it down to https://github.com/Pike/elmo/commit/a0c8264ee24d28e26b8de54f6a45e030fd9406c3.

The idea of having the select_related inside signoff_summary is nice, but doesn't work in shipping.views.signoff.signoff, as I need the actions twice there, and if we subquery with select_related, we don't get the query results cached. Also, the pull request did break tests badly.

Here's the overall diff again, not rebased. I guess I'll just merge the branch when I get an r+.
Attachment #598226 - Attachment is obsolete: true
Attachment #598898 - Flags: review?(peterbe)
(Reporter)

Comment 6

7 years ago
Comment on attachment 598898 [details] [diff] [review]
incorporate tweaked merge request

Review of attachment 598898 [details] [diff] [review]:
-----------------------------------------------------------------

I honestly still don't really fully understand some of the business logic in there (or should I say; many-to-many logic) but I trust you. 

Because all this stuff is quite important we ought to take a look at the test code coverage and make sure it's sufficiently high.

::: apps/shipping/views/__init__.py
@@ +95,4 @@
>      def __getattr__(self, key):  # for dot notation
>          return self[key]
>  
> +    def __setattr__(self, key, value):  # for dot notation setters

I can't spot where the need for this is. Doesn't really matter because the in-line comment is good.

@@ +190,4 @@
>  
>          applications[application].append(run)
> +    # get the tip shortrevs for all our pushes
> +    pushes = map(lambda p: p.id, filter(None, pushes))

if you're not going to use the filter I would suggest a list comprehension thingy instead.
Attachment #598898 - Flags: review?(peterbe) → review+
(Assignee)

Comment 7

7 years ago
Recap from IRC:

we're doing

run.pending = None

etc in the upper part of the code, and without the setattr, that's not making it into the dict part, and run['pending'] raises a KeyError.

The state of my branch landed on develop, and the german page went from some 20s to 3.5.

Peter has some more fixes on his mental list, but from what I read on irc, they're probably better suited for a new bug.

https://github.com/mozilla/elmo/commit/1bffdacb57178a40e182c826bf00aab9d6b9e27b is the merge, FIXED.

Also, I prefer this to be FIXED than anything else as I think the problems that have blocked the next release are wrapped up with this.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Assignee)

Updated

7 years ago
Target Milestone: --- → 2.1

Comment 8

2 years ago
Commit pushed to develop at https://github.com/mozilla/elmo

https://github.com/mozilla/elmo/commit/a1b80935d50433beb0138043d49fb1548f3bc2de
Removing dead code, rs=me

This is a follow-up to bug 713878 and bug 661027, which stopped
using the l10nstats team snippet, and the homepage snippets
alltogether. We never removed the code, though.
You need to log in before you can comment on or make changes to this bug.