Reduce number of database queries made by Scanner Query Rules#25181
Reduce number of database queries made by Scanner Query Rules#25181diox wants to merge 1 commit into
Conversation
Before, using a chunk of 250 versions, each chunk would do: - 251 queries for yara + 4 per match - 1,001 queries for narc + 4 per match After, same chunk of 250 versions: - 3 queries for yara + 1 for all matches - 5 queries for narc + 1 for all matches The main query for each chunk now has a bunch of joins so it's more costly, but given the number of queries it saves the tradeoff should be worth it.
|
FWIW locally using a bunch (~ 2000) of add-ons with real xpis stolen from prod, using rules that matches everything (yara Obviously though this is not a proper test, I don't have the volume, same workers setup, CPU/memory etc prod has. |
| values_from_authors = [ | ||
| author.display_name | ||
| for author in addon.authors.all() | ||
| if author.display_name is not None | ||
| ] |
There was a problem hiding this comment.
Isn't this less efficient than what it replaced?
There was a problem hiding this comment.
Ever so slightly if we look at it in isolation, but iterating over authors.all() allows to benefit from the prefetched queryset set up by the parent method. Ultimately add-on authors should all have a display name anyway (and the vast majority of add-ons only have one author)
| attach_trans_dict( | ||
| Addon, [version.addon for version in qs], field_names=['name', 'summary'] | ||
| ) |
There was a problem hiding this comment.
optimization: collect all the add-ons from all the versions in a set so we're not attaching translations once for each add-on if there are multiple versions from the same add-on. Or is that already happening anyway by magic inside of attach_trans_dict (I skim read that function, it wasn't clear either way)
There was a problem hiding this comment.
Oh, that's a good idea. attach_trans_dict doesn't appear to be checking if we already fetched translations for a given add-on so yes, it could be significant for add-ons with multiple versions.
Instead of collecting in a set() we could just check if translations property is set on the add-on, I'll do that.
| Addon, [version.addon for version in qs], field_names=['name', 'summary'] | ||
| ) | ||
| results = [] | ||
| for version in qs: |
There was a problem hiding this comment.
I'm assuming django is clever enough to not re-fetch the version instances if it's been fetched on line 743
There was a problem hiding this comment.
Yes, if it's the same queryset instance and we aren't using .iterator(), it's all cached on the queryset.
| ) | ||
| # Force waffle switch to be cached to not affect queries count. | ||
| waffle.switch_is_active('use-yara-x') | ||
| with self.assertNumQueries(3): |
There was a problem hiding this comment.
To be certain we could re-run this query with an extra add-on to confirm it's still 3 queries. Similarly the other new tests in this file. (But with the different combinations of tests it's going to be very unlikely to get a false positive anyway)
2 participants
Description
Optimize number of queries made by scanner query rules
Context
Scanner query rules run against a bunch of versions in the database. As we have millions of versions in the database, a rule is divided into chunks that each process up to 250 versions.
Before this change, assuming a chunk of 250 versions, each chunk would execute:
After this change, the same chunk of 250 versions would execute:
The main query for each chunk now has a bunch of joins so it's more costly, but given the number of queries it saves the tradeoff should be worth it.
Fixes mozilla/addons#16346
Testing
We should have robust unit tests covering this. Scanner query rules can be created and triggered through the admin tools: http://olympia.test/en-US/admin/models/scanners/scannerqueryrule/