From 5c6ccc8f0b8038a0c3009c44356ee0bb2ed77e5e Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Thu, 28 May 2015 22:50:52 -0400 Subject: [PATCH 1/9] Add CCX SQL query/mongo read tests --- cms/djangoapps/contentstore/tests/utils.py | 29 +-- .../xmodule/modulestore/tests/utils.py | 30 +++ .../tests/test_field_override_performance.py | 203 ++++++++++++++++++ lms/djangoapps/ccx/tests/test_overrides.py | 1 + 4 files changed, 239 insertions(+), 24 deletions(-) create mode 100644 lms/djangoapps/ccx/tests/test_field_override_performance.py diff --git a/cms/djangoapps/contentstore/tests/utils.py b/cms/djangoapps/contentstore/tests/utils.py index 73f3541441..4c9cd57841 100644 --- a/cms/djangoapps/contentstore/tests/utils.py +++ b/cms/djangoapps/contentstore/tests/utils.py @@ -11,15 +11,16 @@ from django.contrib.auth.models import User from django.test.client import Client from opaque_keys.edx.locations import SlashSeparatedCourseKey, AssetLocation -from contentstore.utils import reverse_url -from student.models import Registration +from contentstore.utils import reverse_url # pylint: disable=import-error +from student.models import Registration # pylint: disable=import-error from xmodule.modulestore.split_mongo.split import SplitMongoModuleStore from xmodule.contentstore.django import contentstore from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase -from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.modulestore.tests.factories import CourseFactory from xmodule.modulestore.xml_importer import import_course_from_xml +from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT @@ -67,7 +68,7 @@ class AjaxEnabledTestClient(Client): return self.get(path, data or {}, follow, HTTP_ACCEPT="application/json", **extra) -class CourseTestCase(ModuleStoreTestCase): +class CourseTestCase(ProceduralCourseTestMixin, ModuleStoreTestCase): """ Base class for Studio tests that require a logged in user and a course. Also provides helper methods for manipulating and verifying the course. @@ -100,26 +101,6 @@ class CourseTestCase(ModuleStoreTestCase): nonstaff.is_authenticated = lambda: authenticate return client, nonstaff - def populate_course(self, branching=2): - """ - Add k chapters, k^2 sections, k^3 verticals, k^4 problems to self.course (where k = branching) - """ - user_id = self.user.id - self.populated_usage_keys = {} - - def descend(parent, stack): - if not stack: - return - - xblock_type = stack[0] - for _ in range(branching): - child = ItemFactory.create(category=xblock_type, parent_location=parent.location, user_id=user_id) - print child.location - self.populated_usage_keys.setdefault(xblock_type, []).append(child.location) - descend(child, stack[1:]) - - descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) - def reload_course(self): """ Reloads the course object from the database diff --git a/common/lib/xmodule/xmodule/modulestore/tests/utils.py b/common/lib/xmodule/xmodule/modulestore/tests/utils.py index 5a62e06710..60994e4993 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/utils.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/utils.py @@ -143,3 +143,33 @@ class MixedSplitTestCase(TestCase): modulestore=self.store, **extra ) + + +class ProceduralCourseTestMixin(object): + """ + Contains methods for testing courses generated procedurally + """ + def populate_course(self, branching=2): + """ + Add k chapters, k^2 sections, k^3 verticals, k^4 problems to self.course (where k = branching) + """ + user_id = self.user.id + self.populated_usage_keys = {} # pylint: disable=attribute-defined-outside-init + + def descend(parent, stack): # pylint: disable=missing-docstring + if not stack: + return + + xblock_type = stack[0] + for _ in range(branching): + child = ItemFactory.create( + category=xblock_type, + parent_location=parent.location, + user_id=user_id + ) + self.populated_usage_keys.setdefault(xblock_type, []).append( + child.location + ) + descend(child, stack[1:]) + + descend(self.course, ['chapter', 'sequential', 'vertical', 'problem']) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py new file mode 100644 index 0000000000..ffc43ae17c --- /dev/null +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -0,0 +1,203 @@ +# coding=UTF-8 +""" +Performance tests for field overrides. +""" +import ddt +import mock + +from courseware.views import progress # pylint: disable=import-error +from datetime import datetime +from django.core.cache import cache +from django.test.client import RequestFactory +from django.test.utils import override_settings +from edxmako.middleware import MakoMiddleware # pylint: disable=import-error +from nose.plugins.attrib import attr +from pytz import UTC +from student.models import CourseEnrollment +from student.tests.factories import UserFactory # pylint: disable=import-error +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, \ + TEST_DATA_SPLIT_MODULESTORE, TEST_DATA_MONGO_MODULESTORE +from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory +from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin + + +@attr('shard_1') +@mock.patch.dict( + 'django.conf.settings.FEATURES', {'ENABLE_XBLOCK_VIEW_ENDPOINT': True} +) +@ddt.ddt +class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, + ModuleStoreTestCase): + """ + Base class for instrumenting SQL queries and Mongo reads for field override + providers. + """ + def setUp(self): + """ + Create a test client, course, and user. + """ + super(FieldOverridePerformanceTestCase, self).setUp() + + self.request_factory = RequestFactory() + self.student = UserFactory.create() + self.request = self.request_factory.get("foo") + self.request.user = self.student + + MakoMiddleware().process_request(self.request) + + # TEST_DATA must be overridden by subclasses, otherwise the test is + # skipped. + self.TEST_DATA = None + + def setup_course(self, size): + grading_policy = { + "GRADER": [ + { + "drop_count": 2, + "min_count": 12, + "short_label": "HW", + "type": "Homework", + "weight": 0.15 + }, + { + "drop_count": 2, + "min_count": 12, + "type": "Lab", + "weight": 0.15 + }, + { + "drop_count": 0, + "min_count": 1, + "short_label": "Midterm", + "type": "Midterm Exam", + "weight": 0.3 + }, + { + "drop_count": 0, + "min_count": 1, + "short_label": "Final", + "type": "Final Exam", + "weight": 0.4 + } + ], + "GRADE_CUTOFFS": { + "Pass": 0.5 + } + } + + self.course = CourseFactory.create( + graded=True, + start=datetime.now(UTC), + grading_policy=grading_policy + ) + self.populate_course(size) + + CourseEnrollment.enroll( + self.student, + self.course.id + ) + + def grade_course(self, course): + """ + Renders the progress page for the given course. + """ + return progress( + self.request, + course_id=course.id.to_deprecated_string(), + student_id=self.student.id + ) + + def instrument_course_progress_render(self, dataset_index, queries, reads): + """ + Renders the progress page, instrumenting Mongo reads and SQL queries. + """ + self.setup_course(dataset_index + 1) + + # Clear the cache before measuring + # TODO: remove once django cache is disabled in tests + cache.clear() + with self.assertNumQueries(queries): + with check_mongo_calls(reads): + self.grade_course(self.course) + + def run_if_subclassed(self, test_type, dataset_index): + """ + Run the query/read instrumentation only if TEST_DATA has been + overridden. + """ + if not self.TEST_DATA: + self.skipTest( + "Test not properly configured. TEST_DATA must be overridden " + "by a subclass." + ) + + queries, reads = self.TEST_DATA[test_type][dataset_index] + self.instrument_course_progress_render(dataset_index, queries, reads) + + @ddt.data((0,), (1,), (2,)) + @ddt.unpack + @override_settings( + FIELD_OVERRIDE_PROVIDERS=(), + ) + def test_instrument_without_field_override(self, dataset): + """ + Test without any field overrides. + """ + self.run_if_subclassed('no_overrides', dataset) + + @ddt.data((0,), (1,), (2,)) + @ddt.unpack + @override_settings( + FIELD_OVERRIDE_PROVIDERS=( + 'ccx.overrides.CustomCoursesForEdxOverrideProvider', + ), + ) + def test_instrument_with_field_override(self, dataset): + """ + Test with the CCX field override enabled. + """ + self.run_if_subclassed('ccx', dataset) + + +class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): + """ + Test cases for instrumenting field overrides against the Mongo modulestore. + """ + MODULESTORE = TEST_DATA_MONGO_MODULESTORE + + def setUp(self): + """ + Set the modulestore and scaffold the test data. + """ + super(TestFieldOverrideMongoPerformance, self).setUp() + + self.TEST_DATA = { + 'no_overrides': [ + (22, 6), (130, 6), (590, 6) + ], + 'ccx': [ + (22, 6), (130, 6), (590, 6) + ], + } + + +class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): + """ + Test cases for instrumenting field overrides against the Split modulestore. + """ + MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + + def setUp(self): + """ + Set the modulestore and scaffold the test data. + """ + super(TestFieldOverrideSplitPerformance, self).setUp() + + self.TEST_DATA = { + 'no_overrides': [ + (22, 4), (130, 19), (590, 84) + ], + 'ccx': [ + (22, 4), (130, 19), (590, 84) + ] + } diff --git a/lms/djangoapps/ccx/tests/test_overrides.py b/lms/djangoapps/ccx/tests/test_overrides.py index 37a7a14d36..e5d78d0d3d 100644 --- a/lms/djangoapps/ccx/tests/test_overrides.py +++ b/lms/djangoapps/ccx/tests/test_overrides.py @@ -1,3 +1,4 @@ +# coding=UTF-8 """ tests for overrides """ From 1e0163a27ae384a4ad73883b0ee439502b68e6ab Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 11:34:49 -0400 Subject: [PATCH 2/9] Make RequestCache.clear_request_cache a classmethod --- common/djangoapps/request_cache/middleware.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/request_cache/middleware.py b/common/djangoapps/request_cache/middleware.py index 72c6d4114e..7690eac5ba 100644 --- a/common/djangoapps/request_cache/middleware.py +++ b/common/djangoapps/request_cache/middleware.py @@ -18,7 +18,11 @@ class RequestCache(object): """ return _request_cache_threadlocal.request - def clear_request_cache(self): + @classmethod + def clear_request_cache(cls): + """ + Empty the request cache. + """ _request_cache_threadlocal.data = {} _request_cache_threadlocal.request = None From 984eb0a436c3e6ea8f59ff24e89265d047b4af17 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 11:35:26 -0400 Subject: [PATCH 3/9] Use the more abstract api for parsing UsageKeys in mongo/base.py --- common/lib/xmodule/xmodule/modulestore/mongo/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 9f62ee5ef4..1e19bf805e 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -324,7 +324,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): """ Convert a single serialized UsageKey string in a ReferenceField into a UsageKey. """ - key = Location.from_string(ref_string) + key = UsageKey.from_string(ref_string) return key.replace(run=self.modulestore.fill_in_run(key.course_key).run) def __setattr__(self, name, value): From 7da8eb1fdb755b2ebd618337f318be56bed3b0e6 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 11:37:33 -0400 Subject: [PATCH 4/9] Delay constructing the set of mongo calls until they're needed --- .../lib/xmodule/xmodule/modulestore/tests/factories.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index 9e1f1ae5c7..e27be9f0ab 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -330,18 +330,18 @@ def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1): yield call_count = sum(mock.call_count for mock in mocks.values()) - calls = pprint.pformat({ - method_name: mock.call_args_list - for method_name, mock in mocks.items() - }) # Assertion errors don't handle multi-line values, so pretty-print to std-out instead if not minimum_calls <= call_count <= maximum_calls: + calls = { + method_name: mock.call_args_list + for method_name, mock in mocks.items() + } print "Expected between {} and {} calls, {} were made. Calls: {}".format( minimum_calls, maximum_calls, call_count, - calls, + pprint.pformat(calls), ) # verify the counter actually worked by ensuring we have counted greater than (or equal to) the minimum calls From 5962c4eb8f46ffb3a746f6bf7bafb2e1b9d955dd Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 11:38:20 -0400 Subject: [PATCH 5/9] Better simulate a request happening in the LMS in FieldOverride performance tests --- .../tests/test_field_override_performance.py | 121 ++++++++---------- 1 file changed, 54 insertions(+), 67 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index ffc43ae17c..7c832ff38c 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -3,18 +3,21 @@ Performance tests for field overrides. """ import ddt +import itertools import mock from courseware.views import progress # pylint: disable=import-error from datetime import datetime -from django.core.cache import cache +from django.core.cache import get_cache from django.test.client import RequestFactory from django.test.utils import override_settings from edxmako.middleware import MakoMiddleware # pylint: disable=import-error from nose.plugins.attrib import attr from pytz import UTC +from request_cache.middleware import RequestCache from student.models import CourseEnrollment from student.tests.factories import UserFactory # pylint: disable=import-error +from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, \ TEST_DATA_SPLIT_MODULESTORE, TEST_DATA_MONGO_MODULESTORE from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory @@ -32,6 +35,11 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, Base class for instrumenting SQL queries and Mongo reads for field override providers. """ + __test__ = False + + # TEST_DATA must be overridden by subclasses + TEST_DATA = None + def setUp(self): """ Create a test client, course, and user. @@ -42,14 +50,14 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, self.student = UserFactory.create() self.request = self.request_factory.get("foo") self.request.user = self.student + self.course = None MakoMiddleware().process_request(self.request) - # TEST_DATA must be overridden by subclasses, otherwise the test is - # skipped. - self.TEST_DATA = None - def setup_course(self, size): + """ + Build a gradable course where each node has `size` children. + """ grading_policy = { "GRADER": [ { @@ -113,50 +121,39 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, """ self.setup_course(dataset_index + 1) - # Clear the cache before measuring - # TODO: remove once django cache is disabled in tests - cache.clear() - with self.assertNumQueries(queries): - with check_mongo_calls(reads): - self.grade_course(self.course) + # Switch to published-only mode to simulate the LMS + with self.settings(MODULESTORE_BRANCH='published-only'): + # Clear the cache before measuring + # We clear the mongo_metadata_inheritance cache so that we can refill it + # with published-only contents. + get_cache('mongo_metadata_inheritance').clear() - def run_if_subclassed(self, test_type, dataset_index): - """ - Run the query/read instrumentation only if TEST_DATA has been - overridden. - """ - if not self.TEST_DATA: - self.skipTest( - "Test not properly configured. TEST_DATA must be overridden " - "by a subclass." - ) + # Refill the metadata inheritance cache + modulestore().get_course(self.course.id, depth=None) - queries, reads = self.TEST_DATA[test_type][dataset_index] - self.instrument_course_progress_render(dataset_index, queries, reads) + # We clear the request cache to simulate a new request in the LMS. + RequestCache.clear_request_cache() - @ddt.data((0,), (1,), (2,)) + with self.assertNumQueries(queries): + with check_mongo_calls(reads): + self.grade_course(self.course) + + @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(3))) @ddt.unpack @override_settings( FIELD_OVERRIDE_PROVIDERS=(), ) - def test_instrument_without_field_override(self, dataset): + def test_field_overrides(self, overrides, dataset_index): """ Test without any field overrides. """ - self.run_if_subclassed('no_overrides', dataset) - - @ddt.data((0,), (1,), (2,)) - @ddt.unpack - @override_settings( - FIELD_OVERRIDE_PROVIDERS=( - 'ccx.overrides.CustomCoursesForEdxOverrideProvider', - ), - ) - def test_instrument_with_field_override(self, dataset): - """ - Test with the CCX field override enabled. - """ - self.run_if_subclassed('ccx', dataset) + providers = { + 'no_overrides': (), + 'ccx': ('ccx.overrides.CustomCoursesForEdxOverrideProvider',) + } + with self.settings(FIELD_OVERRIDE_PROVIDERS=providers[overrides]): + queries, reads = self.TEST_DATA[overrides][dataset_index] + self.instrument_course_progress_render(dataset_index, queries, reads) class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): @@ -164,21 +161,16 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): Test cases for instrumenting field overrides against the Mongo modulestore. """ MODULESTORE = TEST_DATA_MONGO_MODULESTORE + __test__ = True - def setUp(self): - """ - Set the modulestore and scaffold the test data. - """ - super(TestFieldOverrideMongoPerformance, self).setUp() - - self.TEST_DATA = { - 'no_overrides': [ - (22, 6), (130, 6), (590, 6) - ], - 'ccx': [ - (22, 6), (130, 6), (590, 6) - ], - } + TEST_DATA = { + 'no_overrides': [ + (26, 7), (132, 7), (592, 7) + ], + 'ccx': [ + (24, 35), (132, 331), (592, 1507) + ], + } class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): @@ -186,18 +178,13 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): Test cases for instrumenting field overrides against the Split modulestore. """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE + __test__ = True - def setUp(self): - """ - Set the modulestore and scaffold the test data. - """ - super(TestFieldOverrideSplitPerformance, self).setUp() - - self.TEST_DATA = { - 'no_overrides': [ - (22, 4), (130, 19), (590, 84) - ], - 'ccx': [ - (22, 4), (130, 19), (590, 84) - ] - } + TEST_DATA = { + 'no_overrides': [ + (24, 4), (132, 19), (592, 84) + ], + 'ccx': [ + (24, 4), (132, 19), (592, 84) + ] + } From 1422db5cb466dd9b3ea32480b4230db281ba21b3 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 13:23:48 -0400 Subject: [PATCH 6/9] Allow check_sum_of_calls to measure methods as well as pure functions --- .../xmodule/modulestore/tests/factories.py | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/factories.py b/common/lib/xmodule/xmodule/modulestore/tests/factories.py index e27be9f0ab..85d8e7f371 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/factories.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/factories.py @@ -1,3 +1,8 @@ +""" +Factories for use in tests of XBlocks. +""" + +import inspect import pprint import threading from uuid import uuid4 @@ -321,12 +326,25 @@ def check_sum_of_calls(object_, methods, maximum_calls, minimum_calls=1): Instruments the given methods on the given object to verify that the total sum of calls made to the methods falls between minumum_calls and maximum_calls. """ + mocks = { method: Mock(wraps=getattr(object_, method)) for method in methods } - with patch.multiple(object_, **mocks): + if inspect.isclass(object_): + # If the object that we're intercepting methods on is a class, rather than a module, + # then we need to set the method to a real function, so that self gets passed to it, + # and then explicitly pass that self into the call to the mock + # pylint: disable=unnecessary-lambda,cell-var-from-loop + mock_kwargs = { + method: lambda self, *args, **kwargs: mocks[method](self, *args, **kwargs) + for method in methods + } + else: + mock_kwargs = mocks + + with patch.multiple(object_, **mock_kwargs): yield call_count = sum(mock.call_count for mock in mocks.values()) From 67bde5e2e918b8fa0c953b7f0b4f50d17792d443 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 13:24:13 -0400 Subject: [PATCH 7/9] Measure XBlock instantiations in FieldOverride performance tests --- .../tests/test_field_override_performance.py | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 7c832ff38c..f33bd40b78 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -17,10 +17,11 @@ from pytz import UTC from request_cache.middleware import RequestCache from student.models import CourseEnrollment from student.tests.factories import UserFactory # pylint: disable=import-error +from xblock.core import XBlock from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase, \ TEST_DATA_SPLIT_MODULESTORE, TEST_DATA_MONGO_MODULESTORE -from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory +from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, check_sum_of_calls from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin @@ -115,7 +116,7 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, student_id=self.student.id ) - def instrument_course_progress_render(self, dataset_index, queries, reads): + def instrument_course_progress_render(self, dataset_index, queries, reads, xblocks): """ Renders the progress page, instrumenting Mongo reads and SQL queries. """ @@ -136,7 +137,8 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, with self.assertNumQueries(queries): with check_mongo_calls(reads): - self.grade_course(self.course) + with check_sum_of_calls(XBlock, ['__init__'], xblocks): + self.grade_course(self.course) @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(3))) @ddt.unpack @@ -152,8 +154,8 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, 'ccx': ('ccx.overrides.CustomCoursesForEdxOverrideProvider',) } with self.settings(FIELD_OVERRIDE_PROVIDERS=providers[overrides]): - queries, reads = self.TEST_DATA[overrides][dataset_index] - self.instrument_course_progress_render(dataset_index, queries, reads) + queries, reads, xblocks = self.TEST_DATA[overrides][dataset_index] + self.instrument_course_progress_render(dataset_index, queries, reads, xblocks) class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): @@ -165,10 +167,10 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): TEST_DATA = { 'no_overrides': [ - (26, 7), (132, 7), (592, 7) + (26, 7, 19), (132, 7, 131), (592, 7, 537) ], 'ccx': [ - (24, 35), (132, 331), (592, 1507) + (24, 35, 47), (132, 331, 455), (592, 1507, 2037) ], } @@ -182,9 +184,9 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): TEST_DATA = { 'no_overrides': [ - (24, 4), (132, 19), (592, 84) + (24, 4, 9), (132, 19, 54), (592, 84, 215) ], 'ccx': [ - (24, 4), (132, 19), (592, 84) + (24, 4, 9), (132, 19, 54), (592, 84, 215) ] } From e4bc328c3a39442fe3eec525fc1624099db391c2 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Tue, 9 Jun 2015 14:05:01 -0400 Subject: [PATCH 8/9] Reduce the number of queries when walking parents in MongoModuleStore by avoiding cache misses on the data due to missing runs --- common/lib/xmodule/xmodule/modulestore/mongo/base.py | 10 +++++++++- .../ccx/tests/test_field_override_performance.py | 2 +- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 1e19bf805e..829dad0489 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -228,6 +228,14 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): Return an XModule instance for the specified location """ assert isinstance(location, UsageKey) + + if location.run is None: + # self.module_data is keyed on locations that have full run information. + # If the supplied location is missing a run, then we will miss the cache and + # incur an additional query. + # TODO: make module_data a proper class that can handle this itself. + location = location.replace(course_key=self.modulestore.fill_in_run(location.course_key)) + json_data = self.module_data.get(location) if json_data is None: module = self.modulestore.get_item(location, using_descriptor_system=self) @@ -258,7 +266,7 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): else ModuleStoreEnum.Branch.draft_preferred ) if parent_url: - parent = BlockUsageLocator.from_string(parent_url) + parent = self._convert_reference_to_key(parent_url) if not parent and category != 'course': # try looking it up just-in-time (but not if we're working with a root node (course). parent = self.modulestore.get_parent_location( diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index f33bd40b78..6d142015c8 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -170,7 +170,7 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): (26, 7, 19), (132, 7, 131), (592, 7, 537) ], 'ccx': [ - (24, 35, 47), (132, 331, 455), (592, 1507, 2037) + (24, 7, 47), (132, 7, 455), (592, 7, 2037) ], } From b06d256f99cdf9ae39a09131057f685ec8cc9978 Mon Sep 17 00:00:00 2001 From: Calen Pennington Date: Fri, 12 Jun 2015 12:24:35 -0400 Subject: [PATCH 9/9] Clear all caches before measure FieldOverride performance --- .../ccx/tests/test_field_override_performance.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_field_override_performance.py b/lms/djangoapps/ccx/tests/test_field_override_performance.py index 6d142015c8..88f81a42b1 100644 --- a/lms/djangoapps/ccx/tests/test_field_override_performance.py +++ b/lms/djangoapps/ccx/tests/test_field_override_performance.py @@ -8,6 +8,7 @@ import mock from courseware.views import progress # pylint: disable=import-error from datetime import datetime +from django.conf import settings from django.core.cache import get_cache from django.test.client import RequestFactory from django.test.utils import override_settings @@ -124,10 +125,9 @@ class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin, # Switch to published-only mode to simulate the LMS with self.settings(MODULESTORE_BRANCH='published-only'): - # Clear the cache before measuring - # We clear the mongo_metadata_inheritance cache so that we can refill it - # with published-only contents. - get_cache('mongo_metadata_inheritance').clear() + # Clear all caches before measuring + for cache in settings.CACHES: + get_cache(cache).clear() # Refill the metadata inheritance cache modulestore().get_course(self.course.id, depth=None) @@ -167,10 +167,10 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase): TEST_DATA = { 'no_overrides': [ - (26, 7, 19), (132, 7, 131), (592, 7, 537) + (26, 7, 19), (134, 7, 131), (594, 7, 537) ], 'ccx': [ - (24, 7, 47), (132, 7, 455), (592, 7, 2037) + (26, 7, 47), (134, 7, 455), (594, 7, 2037) ], } @@ -184,9 +184,9 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase): TEST_DATA = { 'no_overrides': [ - (24, 4, 9), (132, 19, 54), (592, 84, 215) + (26, 4, 9), (134, 19, 54), (594, 84, 215) ], 'ccx': [ - (24, 4, 9), (132, 19, 54), (592, 84, 215) + (26, 4, 9), (134, 19, 54), (594, 84, 215) ] }