From 2cb2265ffe34b065e0e50a55ba454d7215ec6418 Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Wed, 4 Dec 2019 19:39:18 -0500 Subject: [PATCH 1/2] Add request caching to reduce ORM queries This adds request caching to the following places: * course expiration wrapper (displayed in Units) * offer banner generation (displayed in Units) * get_enrollment * user_by_anonymous_id * youtube_disabled_for_course On a sample course with edx-val enabled, this reduced the queries for a large sequence from 450 to 155. --- common/djangoapps/course_modes/views.py | 1 - common/djangoapps/student/models.py | 29 +++++++++- .../xmodule/video_module/video_module.py | 13 +++-- .../tests/test_field_override_performance.py | 4 +- lms/djangoapps/courseware/tests/test_views.py | 12 ++-- lms/djangoapps/discussion/tests/test_views.py | 16 +++--- .../grades/tests/test_course_grade_factory.py | 16 +++--- .../features/course_duration_limits/access.py | 40 +++++++++++-- .../tests/views/test_course_home.py | 2 +- .../tests/views/test_course_updates.py | 2 +- openedx/features/discounts/utils.py | 57 ++++++++++++++++--- 11 files changed, 147 insertions(+), 45 deletions(-) diff --git a/common/djangoapps/course_modes/views.py b/common/djangoapps/course_modes/views.py index 4848e7a440..ccbed449f9 100644 --- a/common/djangoapps/course_modes/views.py +++ b/common/djangoapps/course_modes/views.py @@ -36,7 +36,6 @@ from openedx.core.djangoapps.embargo import api as embargo_api from openedx.core.djangoapps.enrollments.permissions import ENROLL_IN_COURSE from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.models import CourseDurationLimitConfig -from openedx.features.discounts.utils import get_first_purchase_offer_banner_fragment from openedx.features.discounts.applicability import discount_percentage from student.models import CourseEnrollment from util.db import outer_atomic diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 6df0f65647..ea11664594 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -209,9 +209,17 @@ def user_by_anonymous_id(uid): if uid is None: return None + request_cache = RequestCache('user_by_anonymous_id') + cache_response = request_cache.get_cached_response(uid) + if cache_response.is_found: + return cache_response.value + try: - return User.objects.get(anonymoususerid__anonymous_user_id=uid) + user = User.objects.get(anonymoususerid__anonymous_user_id=uid) + request_cache.set(uid, user) + return user except ObjectDoesNotExist: + request_cache.set(uid, None) return None @@ -1207,13 +1215,24 @@ class CourseEnrollment(models.Model): if user.is_anonymous: return None try: + request_cache = RequestCache('get_enrollment') + if select_related: + cache_key = (user.id, course_key, ','.join(select_related)) + else: + cache_key = (user.id, course_key) + cache_response = request_cache.get_cached_response(cache_key) + if cache_response.is_found: + return cache_response.value + query = cls.objects if select_related is not None: query = query.select_related(*select_related) - return query.get( + enrollment = query.get( user=user, course_id=course_key ) + request_cache.set(cache_key, enrollment) + return enrollment except cls.DoesNotExist: return None @@ -1257,6 +1276,8 @@ class CourseEnrollment(models.Model): This saves immediately. """ + RequestCache('get_enrollment').clear() + activation_changed = False # if is_active is None, then the call to update_enrollment didn't specify # any value, so just leave is_active as it is @@ -1488,6 +1509,8 @@ class CourseEnrollment(models.Model): `skip_refund` can be set to True to avoid the refund process. """ + RequestCache('get_enrollment').clear() + try: record = cls.objects.get(user=user, course_id=course_id) record.update_enrollment(is_active=False, skip_refund=skip_refund) @@ -1509,6 +1532,8 @@ class CourseEnrollment(models.Model): `course_id` is our usual course_id string (e.g. "edX/Test101/2013_Fall) """ + RequestCache('get_enrollment').clear() + try: user = User.objects.get(email=email) return cls.unenroll(user, course_id) diff --git a/common/lib/xmodule/xmodule/video_module/video_module.py b/common/lib/xmodule/xmodule/video_module/video_module.py index 1ad75f14b8..9ca7624555 100644 --- a/common/lib/xmodule/xmodule/video_module/video_module.py +++ b/common/lib/xmodule/xmodule/video_module/video_module.py @@ -22,6 +22,7 @@ from operator import itemgetter import six from django.conf import settings +from edx_django_utils.cache import RequestCache from lxml import etree from opaque_keys.edx.locator import AssetLocator from web_fragments.fragment import Fragment @@ -202,10 +203,14 @@ class VideoBlock( def youtube_disabled_for_course(self): if not self.location.context_key.is_course: return False # Only courses have this flag - if CourseYoutubeBlockedFlag.feature_enabled(self.location.course_key): - return True - else: - return False + request_cache = RequestCache('youtube_disabled_for_course') + cache_response = request_cache.get_cached_response(self.location.context_key) + if cache_response.is_found: + return cache_response.value + + youtube_is_disabled = CourseYoutubeBlockedFlag.feature_enabled(self.location.course_key) + request_cache.set(self.location.context_key, youtube_is_disabled) + return youtube_is_disabled def prioritize_hls(self, youtube_streams, html5_sources): """ diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 0b60ddc345..4cd9db66a9 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -244,7 +244,7 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 32 + QUERY_COUNT = 29 TEST_DATA = { # (providers, course_width, enable_ccx, view_as_ccx): ( # # of sql queries to default, @@ -273,7 +273,7 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): __test__ = True # TODO: decrease query count as part of REVO-28 - QUERY_COUNT = 32 + QUERY_COUNT = 29 TEST_DATA = { ('no_overrides', 1, True, False): (QUERY_COUNT, 3), diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 6f05a576f1..c62882bde9 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -260,8 +260,8 @@ class IndexQueryTestCase(ModuleStoreTestCase): NUM_PROBLEMS = 20 @ddt.data( - (ModuleStoreEnum.Type.mongo, 10, 182), - (ModuleStoreEnum.Type.split, 4, 178), + (ModuleStoreEnum.Type.mongo, 10, 176), + (ModuleStoreEnum.Type.split, 4, 174), ) @ddt.unpack def test_index_query_counts(self, store_type, expected_mongo_query_count, expected_mysql_query_count): @@ -1494,8 +1494,8 @@ class ProgressPageTests(ProgressPageBaseTests): self.assertContains(resp, u"Download Your Certificate") @ddt.data( - (True, 52), - (False, 51) + (True, 49), + (False, 48) ) @ddt.unpack def test_progress_queries_paced_courses(self, self_paced, query_count): @@ -1508,8 +1508,8 @@ class ProgressPageTests(ProgressPageBaseTests): @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) @ddt.data( - (False, 60, 40), - (True, 51, 35) + (False, 57, 37), + (True, 48, 32) ) @ddt.unpack def test_progress_queries(self, enable_waffle, initial, subsequent): diff --git a/lms/djangoapps/discussion/tests/test_views.py b/lms/djangoapps/discussion/tests/test_views.py index 7019c629b6..dd57f67d44 100644 --- a/lms/djangoapps/discussion/tests/test_views.py +++ b/lms/djangoapps/discussion/tests/test_views.py @@ -464,18 +464,18 @@ class SingleThreadQueryCountTestCase(ForumsEnableMixin, ModuleStoreTestCase): # course is outside the context manager that is verifying the number of queries, # and with split mongo, that method ends up querying disabled_xblocks (which is then # cached and hence not queried as part of call_single_thread). - (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 23, 8), - (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 23, 8), + (ModuleStoreEnum.Type.mongo, False, 1, 5, 2, 23, 7), + (ModuleStoreEnum.Type.mongo, False, 50, 5, 2, 23, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, False, 1, 3, 3, 23, 8), - (ModuleStoreEnum.Type.split, False, 50, 3, 3, 23, 8), + (ModuleStoreEnum.Type.split, False, 1, 3, 3, 23, 7), + (ModuleStoreEnum.Type.split, False, 50, 3, 3, 23, 7), # Enabling Enterprise integration should have no effect on the number of mongo queries made. - (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 23, 8), - (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 23, 8), + (ModuleStoreEnum.Type.mongo, True, 1, 5, 2, 23, 7), + (ModuleStoreEnum.Type.mongo, True, 50, 5, 2, 23, 7), # split mongo: 3 queries, regardless of thread response size. - (ModuleStoreEnum.Type.split, True, 1, 3, 3, 23, 8), - (ModuleStoreEnum.Type.split, True, 50, 3, 3, 23, 8), + (ModuleStoreEnum.Type.split, True, 1, 3, 3, 23, 7), + (ModuleStoreEnum.Type.split, True, 50, 3, 3, 23, 7), ) @ddt.unpack def test_number_of_mongo_queries( diff --git a/lms/djangoapps/grades/tests/test_course_grade_factory.py b/lms/djangoapps/grades/tests/test_course_grade_factory.py index 0565bd31e7..cf267b4788 100644 --- a/lms/djangoapps/grades/tests/test_course_grade_factory.py +++ b/lms/djangoapps/grades/tests/test_course_grade_factory.py @@ -100,32 +100,32 @@ class TestCourseGradeFactory(GradeTestBase): with self.assertNumQueries(3), mock_get_score(1, 2): _assert_read(expected_pass=False, expected_percent=0) # start off with grade of 0 - num_queries = 46 + num_queries = 45 with self.assertNumQueries(num_queries), mock_get_score(1, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) - with self.assertNumQueries(4): + with self.assertNumQueries(3): _assert_read(expected_pass=True, expected_percent=0.5) # updated to grade of .5 - num_queries = 8 + num_queries = 7 with self.assertNumQueries(num_queries), mock_get_score(1, 4): grade_factory.update(self.request.user, self.course, force_update_subsections=False) - with self.assertNumQueries(4): + with self.assertNumQueries(3): _assert_read(expected_pass=True, expected_percent=0.5) # NOT updated to grade of .25 - num_queries = 25 + num_queries = 24 with self.assertNumQueries(num_queries), mock_get_score(2, 2): grade_factory.update(self.request.user, self.course, force_update_subsections=True) - with self.assertNumQueries(4): + with self.assertNumQueries(3): _assert_read(expected_pass=True, expected_percent=1.0) # updated to grade of 1.0 - num_queries = 29 + num_queries = 28 with self.assertNumQueries(num_queries), mock_get_score(0, 0): # the subsection now is worth zero grade_factory.update(self.request.user, self.course, force_update_subsections=True) - with self.assertNumQueries(4): + with self.assertNumQueries(3): _assert_read(expected_pass=False, expected_percent=0.0) # updated to grade of 0.0 @patch.dict(settings.FEATURES, {'ASSUME_ZERO_GRADE_IF_ABSENT_FOR_ALL_TESTS': False}) diff --git a/openedx/features/course_duration_limits/access.py b/openedx/features/course_duration_limits/access.py index 9a86b0d9ef..6a0f30e8c2 100644 --- a/openedx/features/course_duration_limits/access.py +++ b/openedx/features/course_duration_limits/access.py @@ -11,6 +11,7 @@ import six from django.utils import timezone from django.utils.translation import get_language from django.utils.translation import ugettext as _ +from edx_django_utils.cache import RequestCache from web_fragments.fragment import Fragment from course_modes.models import CourseMode @@ -229,11 +230,42 @@ def generate_course_expired_message(user, course): def generate_course_expired_fragment(user, course): message = generate_course_expired_message(user, course) if message: - return Fragment(HTML(u"""\ + return generate_fragment_from_message(message) + + +def generate_fragment_from_message(message): + return Fragment(HTML(u"""\
{}
""").format(message)) +def generate_course_expired_fragment_from_key(user, course_key): + """ + Like `generate_course_expired_fragment`, but using a CourseKey instead of + a CourseOverview and using request-level caching. + + Either returns WebFragment to inject XBlock content into, or None if we + shouldn't show a course expired message for this user. + """ + request_cache = RequestCache('generate_course_expired_fragment_from_key') + cache_key = u'message:{},{}'.format(user.id, course_key) + cache_response = request_cache.get_cached_response(cache_key) + if cache_response.is_found: + cached_message = cache_response.value + # In this case, there is no message to display. + if cached_message is None: + return None + return generate_fragment_from_message(cached_message) + + course = CourseOverview.get_from_id(course_key) + message = generate_course_expired_message(user, course) + request_cache.set(cache_key, message) + if message is None: + return None + + return generate_fragment_from_message(message) + + def course_expiration_wrapper(user, block, view, frag, context): # pylint: disable=W0613 """ An XBlock wrapper that prepends a message to the beginning of a vertical if @@ -242,9 +274,9 @@ def course_expiration_wrapper(user, block, view, frag, context): # pylint: disa if block.category != "vertical": return frag - course = CourseOverview.get_from_id(block.course_id) - course_expiration_fragment = generate_course_expired_fragment(user, course) - + course_expiration_fragment = generate_course_expired_fragment_from_key( + user, block.course_id + ) if not course_expiration_fragment: return frag diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 151f726344..c2e7141a22 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -219,7 +219,7 @@ class TestCourseHomePage(CourseHomePageTestCase): # Fetch the view and verify the query counts # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(90, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(74, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_home_url(self.course) self.client.get(url) diff --git a/openedx/features/course_experience/tests/views/test_course_updates.py b/openedx/features/course_experience/tests/views/test_course_updates.py index 0f184f3860..a0b6b4150d 100644 --- a/openedx/features/course_experience/tests/views/test_course_updates.py +++ b/openedx/features/course_experience/tests/views/test_course_updates.py @@ -134,7 +134,7 @@ class TestCourseUpdatesPage(SharedModuleStoreTestCase): # Fetch the view and verify that the query counts haven't changed # TODO: decrease query count as part of REVO-28 - with self.assertNumQueries(53, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): + with self.assertNumQueries(51, table_blacklist=QUERY_COUNT_TABLE_BLACKLIST): with check_mongo_calls(4): url = course_updates_url(self.course) self.client.get(url) diff --git a/openedx/features/discounts/utils.py b/openedx/features/discounts/utils.py index 8f5576ea5a..bf6640ac92 100644 --- a/openedx/features/discounts/utils.py +++ b/openedx/features/discounts/utils.py @@ -6,6 +6,7 @@ from datetime import datetime import six from django.utils.translation import ugettext as _ +from edx_django_utils.cache import RequestCache import pytz from course_modes.models import get_course_prices, format_course_price @@ -32,8 +33,9 @@ def offer_banner_wrapper(user, block, view, frag, context): # pylint: disable=W if block.category != "vertical": return frag - course = CourseOverview.get_from_id(block.course_id) - offer_banner_fragment = get_first_purchase_offer_banner_fragment(user, course) + offer_banner_fragment = get_first_purchase_offer_banner_fragment_from_key( + user, block.course_id + ) if not offer_banner_fragment: return frag @@ -97,11 +99,12 @@ def format_strikeout_price(user, course, base_price=None, check_for_discount=Tru return (HTML(u"{}").format(original_price), False) -def get_first_purchase_offer_banner_fragment(user, course): +def generate_offer_html(user, course): """ - Return an HTML Fragment with First Purcahse Discount message, - which has the discount_expiration_date, price, - discount percentage and a link to upgrade. + Create the actual HTML object with the offer text in it. + + Returns a openedx.core.djangolib.markup.HTML object, or None if the user + should not be shown an offer message. """ if user and not user.is_anonymous and course: now = datetime.now(tz=pytz.UTC).strftime(u"%Y-%m-%d %H:%M:%S%z") @@ -119,7 +122,8 @@ def get_first_purchase_offer_banner_fragment(user, course): offer_message = _(u'{banner_open} Upgrade by {discount_expiration_date} and save {percentage}% ' u'[{strikeout_price}]{span_close}{br}Discount will be automatically applied at checkout. ' u'{a_open}Upgrade Now{a_close}{div_close}') - return Fragment(HTML(offer_message).format( + + message_html = HTML(offer_message).format( a_open=HTML(u'').format( upgrade_link=verified_upgrade_deadline_link(user=user, course=course) ), @@ -134,5 +138,42 @@ def get_first_purchase_offer_banner_fragment(user, course): span_close=HTML(''), div_close=HTML(''), strikeout_price=HTML(format_strikeout_price(user, course, check_for_discount=False)[0]) - )) + ) + return message_html return None + + +def get_first_purchase_offer_banner_fragment(user, course): + """ + Return an HTML Fragment with First Purcahse Discount message, + which has the discount_expiration_date, price, + discount percentage and a link to upgrade. + """ + offer_html = generate_offer_html(user, course) + if offer_html is None: + return None + return Fragment(offer_html) + + +def get_first_purchase_offer_banner_fragment_from_key(user, course_key): + """ + Like `get_first_purchase_offer_banner_fragment`, but using a CourseKey + instead of a CourseOverview and using request-level caching. + + Either returns WebFragment to inject XBlock content into, or None if we + shouldn't show a first purchase offer message for this user. + """ + request_cache = RequestCache('get_first_purchase_offer_banner_fragment_from_key') + cache_key = u'html:{},{}'.format(user.id, course_key) + cache_response = request_cache.get_cached_response(cache_key) + if cache_response.is_found: + cached_html = cache_response.value + if cached_html is None: + return None + return Fragment(cached_html) + + course = CourseOverview.get_from_id(course_key) + offer_html = generate_offer_html(user, course) + request_cache.set(cache_key, offer_html) + + return Fragment(offer_html) From ad5cc3d5d9c3ea98c39c35171ca5726cbd689b8e Mon Sep 17 00:00:00 2001 From: David Ormsbee Date: Mon, 9 Dec 2019 09:31:53 -0500 Subject: [PATCH 2/2] Request cache context processor output. In Django template rendering, context processors only run once. But when we do template rendering through edxmako (which we do for each and every web fragment/XBlock), we can end up having hundreds of invocations of templates and run context processors for each separately. This removes that work. --- common/djangoapps/edxmako/template.py | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/edxmako/template.py b/common/djangoapps/edxmako/template.py index a9b8016deb..92ae175dfa 100644 --- a/common/djangoapps/edxmako/template.py +++ b/common/djangoapps/edxmako/template.py @@ -15,6 +15,7 @@ from django.conf import settings from django.template import Context, engines +from edx_django_utils.cache import RequestCache from mako.template import Template as MakoTemplate from six import text_type @@ -45,9 +46,27 @@ class Template(object): """ This takes a render call with a context (from Django) and translates it to a render call on the mako template. + + When rendering a large sequence of XBlocks, we may end up rendering + hundreds of small templates. Even if context processors aren't very + expensive individually, they will quickly add up in that situation. To + help guard against this, we do context processing once for a given + request and then cache it. """ context_object = self._get_context_object(request) - context_dictionary = self._get_context_processors_output_dict(context_object) + + request_cache = RequestCache('context_processors') + cache_response = request_cache.get_cached_response('cp_output') + if cache_response.is_found: + context_dictionary = dict(cache_response.value) + else: + context_dictionary = self._get_context_processors_output_dict(context_object) + # The context_dictionary is later updated with template specific + # variables. There are potentially hundreds of calls to templates + # rendering and we don't want them to interfere with each other, so + # we store the items() from the output of the context processors and + # and recreate a new dict every time we pull from the cache. + request_cache.set('cp_output', context_dictionary.items()) if isinstance(context, Context): context_dictionary.update(context.flatten())