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

Bedrock performance tooling and initial optimisations #15513

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion bedrock/base/geo.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,20 @@
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

from django.conf import settings
from django.core.cache import cache

from product_details import product_details


def valid_country_code(country):
codes = product_details.get_regions("en-US").keys()
_key = f"valid_country_codes_for_{country}"

codes = cache.get(_key)

if not codes:
codes = product_details.get_regions("en-US").keys()
cache.set(_key, codes, timeout=settings.CACHE_TIME_MED)

if country and country.lower() in codes:
return country.upper()

Expand Down
48 changes: 37 additions & 11 deletions bedrock/careers/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@
from datetime import datetime
from itertools import chain

from django.conf import settings
from django.core.cache import cache
from django.db import models
from django.urls import reverse

Expand Down Expand Up @@ -37,28 +39,52 @@ def __str__(self):
def get_absolute_url(self):
return reverse("careers.position", kwargs={"source": self.source, "job_id": self.job_id})

@classmethod
def _get_cache_key(cls, name):
return f"careers_position__{name}"

@property
def location_list(self):
return sorted(self.location.split(","))
_key = self._get_cache_key("location_list")
location_list = cache.get(_key)
if location_list is None:
location_list = sorted(self.location.split(","))
cache.set(_key, location_list, settings.CACHE_TIME_LONG)
return location_list

@classmethod
def position_types(cls):
return sorted(set(cls.objects.values_list("position_type", flat=True)))
_key = cls._get_cache_key("position_types")
position_types = cache.get(_key)
if position_types is None:
position_types = sorted(set(cls.objects.values_list("position_type", flat=True)))
cache.set(_key, position_types, settings.CACHE_TIME_LONG)
return position_types

@classmethod
def locations(cls):
return sorted(
{
location.strip()
for location in chain(
*[locations.split(",") for locations in cls.objects.exclude(job_locations="Remote").values_list("job_locations", flat=True)]
)
}
)
_key = cls._get_cache_key("locations")
locations = cache.get(_key)
if locations is None:
locations = sorted(
{
location.strip()
for location in chain(
*[locations.split(",") for locations in cls.objects.exclude(job_locations="Remote").values_list("job_locations", flat=True)]
)
}
)
cache.set(_key, locations, settings.CACHE_TIME_LONG)
return locations

@classmethod
def categories(cls):
return sorted(set(cls.objects.values_list("department", flat=True)))
_key = cls._get_cache_key("categories")
categories = cache.get(_key)
if categories is None:
categories = sorted(set(cls.objects.values_list("department", flat=True)))
cache.set(_key, categories, settings.CACHE_TIME_LONG)
return categories

@property
def cover(self):
Expand Down
6 changes: 6 additions & 0 deletions bedrock/careers/tests/test_forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,18 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.


from django.core.cache import cache

from bedrock.careers.forms import PositionFilterForm
from bedrock.careers.tests import PositionFactory
from bedrock.mozorg.tests import TestCase


class PositionFilterFormTests(TestCase):
def setUp(self):
cache.clear()

def test_dynamic_position_type_choices(self):
"""
The choices for the position_type field should be dynamically
Expand Down
5 changes: 5 additions & 0 deletions bedrock/careers/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

from django.core.cache import cache

from bedrock.careers.models import Position
from bedrock.careers.tests import PositionFactory
from bedrock.mozorg.tests import TestCase


class TestPositionModel(TestCase):
def setUp(self):
cache.clear()

Copy link
Member

Choose a reason for hiding this comment

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

for bonus points you could test that the db call doesn't get triggered on a 2nd call to the wrapped methods.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah - good call. Time to break out assertNumQueries!

def test_location_list(self):
PositionFactory(location="San Francisco,Portland")
pos = Position.objects.get()
Expand Down
3 changes: 3 additions & 0 deletions bedrock/careers/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

from django.core.cache import cache

from bedrock.careers.tests import PositionFactory
from bedrock.careers.utils import generate_position_meta_description
from bedrock.mozorg.tests import TestCase


class GeneratePositionMetaDescriptionTests(TestCase):
def setUp(self):
cache.clear()
self.position = PositionFactory(title="Bowler", position_type="Full time", location="Los Angeles,Ralphs")

def test_position_type_consonant_beginning(self):
Expand Down
11 changes: 10 additions & 1 deletion bedrock/careers/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

from django.conf import settings
from django.core.cache import cache
from django.http.response import Http404
from django.shortcuts import get_object_or_404
from django.views.generic import DetailView, ListView
Expand Down Expand Up @@ -62,10 +64,17 @@ class BenefitsView(L10nTemplateView):


class PositionListView(LangFilesMixin, RequireSafeMixin, ListView):
queryset = Position.objects.exclude(job_locations="Remote")
template_name = "careers/listings.html"
context_object_name = "positions"

def get_queryset(self):
_key = "careers_position_listing_qs"
qs = cache.get(_key)
if qs is None:
qs = Position.objects.exclude(job_locations="Remote")
cache.set(_key, qs, settings.CACHE_TIME_SHORT)
return qs
Copy link
Member

Choose a reason for hiding this comment

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

Should we cache the queryset or cache the list of objects?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally yeah, we'd cache the objects, not the qs, but it's a bit more fiddly than I expected.

Because these are CBVs, cacheing the queryset in get_queryset is easier to do (and to discover) than cacheing the Position objects themselves, because there's no clean way to slot in cacheing when the view sets the object_list attribute that I can see

Later, even though we don't use pagination in these pages, the list view runs the data through paginate_queryset() which currently (Django 4.2.x) takes a queryset arg but treats it as a general iterable -- all of which means we could make get_queryset return a list of objects (from the cache), not a QuerySet, but it kind of muddies things.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for sharing that. It's a bit more complex than first glance. I'm actually a bit surprised we can cache the queryset objects, which seems more complex. I suppose they get pickled? But this is fine with me, I was mostly curious about your thoughts on the consideration. Thanks!


def get_context_data(self, **kwargs):
context = super().get_context_data(**kwargs)
context["form"] = PositionFilterForm()
Expand Down
10 changes: 9 additions & 1 deletion bedrock/newsletter/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

from django.conf import settings
from django.core.cache import cache

import basket

from bedrock.newsletter.models import Newsletter
Expand All @@ -12,7 +15,12 @@ def get_newsletters():
Keys are the internal keys we use to designate newsletters to basket.
Values are dictionaries with the remaining newsletter information.
"""
return Newsletter.objects.serialize()
_key = "serialized_newsletters"
serialized_newsletters = cache.get(_key)
if serialized_newsletters is None:
serialized_newsletters = Newsletter.objects.serialize()
cache.set(_key, serialized_newsletters, timeout=settings.CACHE_TIME_LONG)
return serialized_newsletters


def get_languages_for_newsletters(newsletters=None):
Expand Down
15 changes: 14 additions & 1 deletion bedrock/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,16 @@ def data_path(*args):
"image_renditions": {"URL": f"{REDIS_URL}/0"},
}

CACHE_TIME_SHORT = 60 * 10 # 10 mins
CACHE_TIME_MED = 60 * 60 # 1 hour
CACHE_TIME_LONG = 60 * 60 * 6 # 6 hours


CACHES = {
"default": {
"BACKEND": "bedrock.base.cache.SimpleDictCache",
"LOCATION": "default",
"TIMEOUT": 600,
"TIMEOUT": CACHE_TIME_SHORT,
"OPTIONS": {
"MAX_ENTRIES": 5000,
"CULL_FREQUENCY": 4, # 1/4 entries deleted if max reached
Expand Down Expand Up @@ -2424,3 +2429,11 @@ def lazy_wagtail_langs():
# doesn't exist, we get `None` back from `switch_is_active`.
WAFFLE_SWITCH_DEFAULT = None
WAFFLE_CREATE_MISSING_SWITCHES = False

# Django-silk for performance profiling
if ENABLE_DJANGO_SILK := config("ENABLE_DJANGO_SILK", default="False", parser=bool):
print("Django-Silk profiling enabled - go to http://localhost:8000/silk/ to view metrics")
INSTALLED_APPS.append("silk")
MIDDLEWARE.insert(0, "silk.middleware.SilkyMiddleware")
SUPPORTED_NONLOCALES.append("silk")
SILKY_PYTHON_PROFILER = config("SILKY_PYTHON_PROFILER", default="False", parser=bool)
3 changes: 3 additions & 0 deletions bedrock/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@
path("_internal_draft_preview/", include(wagtaildraftsharing_urls)), # ONLY available in CMS mode
)

if settings.ENABLE_DJANGO_SILK:
urlpatterns += [path("silk/", include("silk.urls", namespace="silk"))]

if settings.DEFAULT_FILE_STORAGE == "django.core.files.storage.FileSystemStorage":
# Serve media files from Django itself - production won't use this
from django.urls import re_path
Expand Down
96 changes: 96 additions & 0 deletions profiling/hit_popular_pages.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at https://mozilla.org/MPL/2.0/.

"""Request a selection of pages that are populat on www.m.o from your local
runserver, so that django-silk can capture performance info on them.

Usage:

1. In your .env set ENABLE_DJANGO_SILK=True
2. Start your runserver on port 8000
3. python profiling/hit_popular_pages.py
3. View results at http://localhost:8000/silk/

"""

import sys
import time

import requests

paths = [
"/en-US/firefox/126.0/whatsnew/",
"/en-US/firefox/",
"/en-US/firefox/windows/",
"/en-US/firefox/new/?reason=manual-update",
"/en-US/firefox/download/thanks/",
"/en-US/firefox/new/?reason=outdated",
"/en-US/firefox/features/",
"/en-US/firefox/all/",
"/en-US/firefox/welcome/18/",
"/en-US/",
"/en-US/firefox/installer-help/?channel=release&installer_lang=en-US",
"/en-US/firefox/download/thanks/?s=direct",
"/en-US/firefox/welcome/19/",
"/en-US/firefox/enterprise/?reason=manual-update",
"/en-US/products/vpn/",
"/en-US/firefox/browsers/windows-64-bit/",
"/en-US/firefox/mac/",
"/en-US/about/",
"/en-US/firefox/android/124.0/releasenotes/",
"/en-US/firefox/browsers/mobile/get-app/",
"/en-US/firefox/browsers/",
"/en-US/firefox/nightly/firstrun/",
"/en-US/firefox/developer/",
"/en-US/account/",
"/en-US/contribute/",
"/en-US/firefox/browsers/mobile/android/",
"/en-US/privacy/archive/firefox-fire-tv/2023-06/",
"/en-US/firefox/121.0/system-requirements/",
"/en-US/firefox/browsers/mobile/",
"/en-US/firefox/releases/",
"/en-US/MPL/",
"/en-US/firefox/enterprise/",
"/en-US/security/advisories/",
"/en-US/firefox/browsers/what-is-a-browser/",
"/en-US/firefox/channel/desktop/?reason=manual-update",
"/en-US/firefox/pocket/",
"/en-US/firefox/channel/desktop/",
"/en-US/firefox/welcome/17b/",
"/en-US/firefox/welcome/17c/",
"/en-US/firefox/welcome/17a/",
"/en-US/firefox/set-as-default/thanks/",
"/en-US/careers/listings/",
"/en-US/firefox/browsers/chromebook/",
"/en-US/firefox/nothing-personal/",
"/en-US/newsletter/existing/",
"/en-US/about/legal/terms/firefox/",
"/en-US/firefox/linux/",
"/en-US/firefox/browsers/mobile/focus/",
"/en-US/products/vpn/download/",
"/en-US/about/manifesto/",
"/en-US/stories/joy-of-color/",
"/en-US/contact/",
"/en-US/about/legal/defend-mozilla-trademarks/",
]


def _log(*args):
sys.stdout.write("\n".join(args))


def hit_pages(paths, times=3):
_base_url = "http://localhost:8000"

for path in paths:
for _ in range(times):
time.sleep(0.5)
url = f"{_base_url}{path}"
requests.get(url)

_log("All done")


if __name__ == "__main__":
hit_pages(paths)
1 change: 1 addition & 0 deletions requirements/dev.in
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

bpython==0.24
braceexpand==0.1.7
django-silk==5.3.1
factory-boy==3.3.1
freezegun==1.5.1
markdown-it-py>=2.2.0
Expand Down
Loading