Addons: improve performance when versions have downloads (#12360)

- This call for getting the project domain wasn't cached, so it was
creating a new resolver for each download type of each version,
resulting in several queries being done. This was extremely slow on
.com, where the resolver needs to check for the organization
subscription. Now the resolver is shared across all calls.
- The resolver cache was updated to not have a limit, so we can cache
more results per instance.
- When getting the version from a project, we were calling
Versions.objects instead of projects.versions, when using the later, the
project object will be shared across all versions, when using the
former, a new instance will be created for each version, which is slow
and doesn't preserve any cached attributes. We should remove the
`project` option from the version querysets, so we are forced to always
use project.versions (will do that in another PR).

All these improvements resulted in going down from 71 queries for a
project with 10 versions with downloads to just 13, and this number is
constant, if more versions are added, we will always have 13 queries.

Author: stsewd

readthedocs/api/v3/serializers.py

@@ -362,7 +362,7 @@ def __init__(self, *args, resolver=None, **kwargs):
         super().__init__(*args, **kwargs)
 
     def get_downloads(self, obj):
-        downloads = obj.get_downloads()
+        downloads = obj.get_downloads(resolver=self.resolver)
         data = {}
 
         for k, v in downloads.items():

readthedocs/builds/models.py

@@ -489,7 +489,7 @@ def get_subdomain_url(self):
             external=external,
         )
 
-    def get_downloads(self, pretty=False):
+    def get_downloads(self, pretty=False, resolver=None):
         project = self.project
         data = {}
 
@@ -500,17 +500,20 @@ def prettify(k):
             data[prettify("PDF")] = project.get_production_media_url(
                 "pdf",
                 self.slug,
+                resolver=resolver,
             )
 
         if self.has_htmlzip:
             data[prettify("HTML")] = project.get_production_media_url(
                 "htmlzip",
                 self.slug,
+                resolver=resolver,
             )
         if self.has_epub:
             data[prettify("Epub")] = project.get_production_media_url(
                 "epub",
                 self.slug,
+                resolver=resolver,
             )
         return data
 

readthedocs/core/resolver.py

@@ -1,6 +1,6 @@
 """URL resolver for documentation."""
 
-from functools import lru_cache
+from functools import cache
 from urllib.parse import urlunparse
 
 import structlog
@@ -56,6 +56,16 @@ class Resolver:
         /docs/<project_slug>/projects/<subproject_slug>/<lang>/<version>/<filename>
         # Subproject Single Version
         /docs/<project_slug>/projects/<subproject_slug>/<filename>
+
+    .. note::
+
+       Several methods ara cached per each instance of the resolver
+       to avoid hitting the database multiple times for the same project.
+
+       A global instance of the resolver shouldn't be used,
+       as resources can change, and results from the resolver will be out of date.
+       Instead, a shared instance of the resolver should be used
+       when doing multiple resolutions for the same set of projects/versions.
     """
 
     def base_resolve_path(
@@ -169,7 +179,7 @@ def resolve_project(self, project, filename="/"):
         protocol = "https" if use_https else "http"
         return urlunparse((protocol, domain, filename, "", "", ""))
 
-    @lru_cache(maxsize=1)
+    @cache
     def _get_project_domain(self, project, external_version_slug=None, use_canonical_domain=True):
         """
         Get the domain from where the documentation of ``project`` is served from.
@@ -178,7 +188,7 @@ def _get_project_domain(self, project, external_version_slug=None, use_canonical
         :param bool use_canonical_domain: If `True` use its canonical custom domain if available.
         :returns: Tuple of ``(domain, use_https)``.
 
-        Note that we are using ``lru_cache`` decorator on this function.
+        Note that we are using ``cache`` decorator on this function.
         This is useful when generating the flyout addons response since we call
         ``resolver.resolve`` multi times for the same ``Project``.
         This cache avoids hitting the DB to get the canonical custom domain over and over again.
@@ -267,7 +277,7 @@ def get_subproject_url_prefix(self, project, external_version_slug=None):
         path = project.subproject_prefix
         return urlunparse((protocol, domain, path, "", "", ""))
 
-    @lru_cache(maxsize=1)
+    @cache
     def _get_canonical_project(self, project):
         """
         Get the parent project and subproject relationship from the canonical project of `project`.
@@ -339,7 +349,7 @@ def _get_project_subdomain(self, project):
         subdomain_slug = project.slug.replace("_", "-")
         return "{}.{}".format(subdomain_slug, settings.PUBLIC_DOMAIN)
 
-    @lru_cache(maxsize=1)
+    @cache
     def _is_external(self, project, version_slug):
         type_ = project.versions.values_list("type", flat=True).filter(slug=version_slug).first()
         return type_ == EXTERNAL

readthedocs/projects/models.py

@@ -763,11 +763,11 @@ def get_storage_path(self, type_, version_slug=LATEST, include_file=True, versio
             )
         return folder_path
 
-    def get_production_media_url(self, type_, version_slug):
+    def get_production_media_url(self, type_, version_slug, resolver=None):
         """Get the URL for downloading a specific media file."""
         # Use project domain for full path --same domain as docs
         # (project-slug.{PUBLIC_DOMAIN} or docs.project.com)
-        domain = self.subdomain()
+        domain = self.subdomain(resolver=resolver)
 
         # NOTE: we can't use ``reverse('project_download_media')`` here
         # because this URL only exists in El Proxito and this method is
@@ -885,20 +885,20 @@ def supports_translations(self):
         """Return whether or not this project supports translations."""
         return self.versioning_scheme == MULTIPLE_VERSIONS_WITH_TRANSLATIONS
 
-    def subdomain(self, use_canonical_domain=True):
+    def subdomain(self, use_canonical_domain=True, resolver=None):
         """Get project subdomain from resolver."""
-        return Resolver().get_domain_without_protocol(
-            self, use_canonical_domain=use_canonical_domain
-        )
+        resolver = resolver or Resolver()
+        return resolver.get_domain_without_protocol(self, use_canonical_domain=use_canonical_domain)
 
-    def get_downloads(self):
+    def get_downloads(self, resolver=None):
         downloads = {}
         default_version = self.get_default_version()
 
         for type_ in ("htmlzip", "epub", "pdf"):
             downloads[type_] = self.get_production_media_url(
                 type_,
                 default_version,
+                resolver=resolver,
             )
 
         return downloads

readthedocs/proxito/tests/test_hosting.py

@@ -384,6 +384,36 @@ def test_flyout_downloads(self):
         }
         assert r.json()["versions"]["current"]["downloads"] == expected
 
+    def test_number_of_queries_versions_with_downloads(self):
+        for i in range(10):
+            fixture.get(
+                Version,
+                project=self.project,
+                privacy_level=PUBLIC,
+                slug=f"offline-{i}",
+                verbose_name=f"offline-{i}",
+                built=True,
+                has_pdf=True,
+                has_epub=True,
+                has_htmlzip=True,
+                active=True,
+            )
+
+        with self.assertNumQueries(13):
+            r = self.client.get(
+                reverse("proxito_readthedocs_docs_addons"),
+                {
+                    "url": "https://project.dev.readthedocs.io/en/offline/",
+                    "client-version": "0.6.0",
+                    "api-version": "1.0.0",
+                },
+                secure=True,
+                headers={
+                    "host": "project.dev.readthedocs.io",
+                },
+            )
+            assert r.status_code == 200
+
     def test_flyout_single_version_project(self):
         self.version.has_pdf = True
         self.version.has_epub = True
@@ -1042,7 +1072,7 @@ def test_file_tree_diff(self, get_manifest):
                 ],
             ),
         ]
-        with self.assertNumQueries(22):
+        with self.assertNumQueries(19):
             r = self.client.get(
                 reverse("proxito_readthedocs_docs_addons"),
                 {

readthedocs/proxito/views/hosting.py

@@ -22,6 +22,7 @@
 from readthedocs.api.v3.serializers import RelatedProjectSerializer
 from readthedocs.api.v3.serializers import VersionSerializer
 from readthedocs.builds.constants import BUILD_STATE_FINISHED
+from readthedocs.builds.constants import INTERNAL
 from readthedocs.builds.constants import LATEST
 from readthedocs.builds.models import Build
 from readthedocs.builds.models import Version
@@ -307,7 +308,9 @@ def _get_versions(self, request, project):
         - They are active
         - They are not hidden
         """
-        return Version.internal.public(
+        # NOTE: Use project.versions, not Version.objects,
+        # so all results share the same instance of project.
+        return project.versions(manager=INTERNAL).public(
             project=project,
             user=request.user,
             only_active=True,
@@ -340,9 +343,7 @@ def _v1(self, project, version, build, filename, url, request):
         versions_active_built_not_hidden = Version.objects.none()
         sorted_versions_active_built_not_hidden = Version.objects.none()
 
-        versions_active_built_not_hidden = (
-            self._get_versions(request, project).select_related("project").order_by("-slug")
-        )
+        versions_active_built_not_hidden = self._get_versions(request, project).order_by("-slug")
         sorted_versions_active_built_not_hidden = versions_active_built_not_hidden
         if not project.supports_multiple_versions:
             # Return only one version when the project doesn't support multiple versions.