Grading followup

1. update query counts with more accuracy
2. don't use defaultdict in block_structure

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -16,6 +16,7 @@ from django.core.cache import caches
 from django.test.client import RequestFactory
 from django.test.utils import override_settings
 from nose.plugins.attrib import attr
+from opaque_keys.edx.keys import CourseKey
 from pytz import UTC
 from request_cache.middleware import RequestCache
 from student.models import CourseEnrollment
@@ -23,7 +24,7 @@ from student.tests.factories import UserFactory
 from xblock.core import XBlock
 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_range, CourseFactory, check_sum_of_calls
+from xmodule.modulestore.tests.factories import check_mongo_calls, CourseFactory, check_sum_of_calls
 from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin
 from ccx_keys.locator import CCXLocator
 from lms.djangoapps.ccx.tests.factories import CcxFactory
@@ -125,15 +126,12 @@ class FieldOverridePerformanceTestCase(FieldOverrideTestMixin, ProceduralCourseT
             self.student,
             course_key
         )
+        return CourseKey.from_string(unicode(course_key))
 
-    def grade_course(self, course, view_as_ccx):
+    def grade_course(self, course_key):
         """
         Renders the progress page for the given course.
         """
-        course_key = course.id
-        if view_as_ccx:
-            course_key = CCXLocator.from_course_locator(course_key, self.ccx.id)
-
         return progress(
             self.request,
             course_id=unicode(course_key),
@@ -145,7 +143,7 @@ class FieldOverridePerformanceTestCase(FieldOverrideTestMixin, ProceduralCourseT
         Assert that mongodb is queried ``calls`` times in the surrounded
         context.
         """
-        return check_mongo_calls_range(max_finds=calls)
+        return check_mongo_calls(calls)
 
     def assertXBlockInstantiations(self, instantiations):
         """
@@ -156,12 +154,12 @@ class FieldOverridePerformanceTestCase(FieldOverrideTestMixin, ProceduralCourseT
 
     def instrument_course_progress_render(
             self, course_width, enable_ccx, view_as_ccx,
-            default_queries, history_queries, reads, xblocks
+            sql_queries, mongo_reads,
     ):
         """
         Renders the progress page, instrumenting Mongo reads and SQL queries.
         """
-        self.setup_course(course_width, enable_ccx, view_as_ccx)
+        course_key = self.setup_course(course_width, enable_ccx, view_as_ccx)
 
         # Switch to published-only mode to simulate the LMS
         with self.settings(MODULESTORE_BRANCH='published-only'):
@@ -170,7 +168,7 @@ class FieldOverridePerformanceTestCase(FieldOverrideTestMixin, ProceduralCourseT
                 caches[cache].clear()
 
             # Refill the metadata inheritance cache
-            get_course_in_cache(self.course.id)
+            get_course_in_cache(course_key)
 
             # We clear the request cache to simulate a new request in the LMS.
             RequestCache.clear_request_cache()
@@ -179,11 +177,11 @@ class FieldOverridePerformanceTestCase(FieldOverrideTestMixin, ProceduralCourseT
             # can actually take affect.
             OverrideFieldData.provider_classes = None
 
-            with self.assertNumQueries(default_queries, using='default'):
-                with self.assertNumQueries(history_queries, using='student_module_history'):
-                    with self.assertMongoCallCount(reads):
-                        with self.assertXBlockInstantiations(xblocks):
-                            self.grade_course(self.course, view_as_ccx)
+            with self.assertNumQueries(sql_queries, using='default'):
+                with self.assertNumQueries(0, using='student_module_history'):
+                    with self.assertMongoCallCount(mongo_reads):
+                        with self.assertXBlockInstantiations(1):
+                            self.grade_course(course_key)
 
     @ddt.data(*itertools.product(('no_overrides', 'ccx'), range(1, 4), (True, False), (True, False)))
     @ddt.unpack
@@ -212,11 +210,11 @@ class FieldOverridePerformanceTestCase(FieldOverrideTestMixin, ProceduralCourseT
             XBLOCK_FIELD_DATA_WRAPPERS=['lms.djangoapps.courseware.field_overrides:OverrideModulestoreFieldData.wrap'],
             MODULESTORE_FIELD_OVERRIDE_PROVIDERS=providers[overrides],
         ):
-            default_queries, history_queries, reads, xblocks = self.TEST_DATA[
+            sql_queries, mongo_reads = self.TEST_DATA[
                 (overrides, course_width, enable_ccx, view_as_ccx)
             ]
             self.instrument_course_progress_render(
-                course_width, enable_ccx, view_as_ccx, default_queries, history_queries, reads, xblocks
+                course_width, enable_ccx, view_as_ccx, sql_queries, mongo_reads,
             )
 
 
@@ -230,28 +228,20 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
     TEST_DATA = {
         # (providers, course_width, enable_ccx, view_as_ccx): (
         #     # of sql queries to default,
-        #     # sql queries to student_module_history,
         #     # of mongo queries,
-        #     # of xblocks
         # )
-        ('no_overrides', 1, True, False): (34, 0, 6, 1),
-        ('no_overrides', 2, True, False): (40, 0, 6, 1),
-        ('no_overrides', 3, True, False): (50, 0, 6, 1),
-        ('ccx', 1, True, False): (34, 0, 6, 1),
-        ('ccx', 2, True, False): (40, 0, 6, 1),
-        ('ccx', 3, True, False): (50, 0, 6, 1),
-        ('ccx', 1, True, True): (47, 0, 6, 1),
-        ('ccx', 2, True, True): (40, 0, 6, 1),
-        ('ccx', 3, True, True): (50, 0, 6, 1),
-        ('no_overrides', 1, False, False): (34, 0, 6, 1),
-        ('no_overrides', 2, False, False): (40, 0, 6, 1),
-        ('no_overrides', 3, False, False): (50, 0, 6, 1),
-        ('ccx', 1, False, False): (34, 0, 6, 1),
-        ('ccx', 2, False, False): (40, 0, 6, 1),
-        ('ccx', 3, False, False): (50, 0, 6, 1),
-        ('ccx', 1, False, True): (47, 0, 6, 1),
-        ('ccx', 2, False, True): (40, 0, 6, 1),
-        ('ccx', 3, False, True): (50, 0, 6, 1),
+        ('no_overrides', 1, True, False): (34, 6),
+        ('no_overrides', 2, True, False): (40, 6),
+        ('no_overrides', 3, True, False): (50, 6),
+        ('ccx', 1, True, False): (34, 6),
+        ('ccx', 2, True, False): (40, 6),
+        ('ccx', 3, True, False): (50, 6),
+        ('no_overrides', 1, False, False): (34, 6),
+        ('no_overrides', 2, False, False): (40, 6),
+        ('no_overrides', 3, False, False): (50, 6),
+        ('ccx', 1, False, False): (34, 6),
+        ('ccx', 2, False, False): (40, 6),
+        ('ccx', 3, False, False): (50, 6),
     }
 
 
@@ -263,22 +253,19 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        ('no_overrides', 1, True, False): (34, 0, 4, 1),
-        ('no_overrides', 2, True, False): (40, 0, 19, 1),
-        ('no_overrides', 3, True, False): (50, 0, 84, 1),
-        ('ccx', 1, True, False): (34, 0, 4, 1),
-        ('ccx', 2, True, False): (40, 0, 19, 1),
-        ('ccx', 3, True, False): (50, 0, 84, 1),
-        ('ccx', 1, True, True): (35, 0, 5, 6),
-        ('ccx', 2, True, True): (41, 0, 20, 47),
-        ('ccx', 3, True, True): (51, 0, 85, 202),
-        ('no_overrides', 1, False, False): (34, 0, 4, 1),
-        ('no_overrides', 2, False, False): (40, 0, 19, 1),
-        ('no_overrides', 3, False, False): (50, 0, 84, 1),
-        ('ccx', 1, False, False): (34, 0, 4, 1),
-        ('ccx', 2, False, False): (40, 0, 19, 1),
-        ('ccx', 3, False, False): (50, 0, 84, 1),
-        ('ccx', 1, False, True): (46, 0, 4, 1),
-        ('ccx', 2, False, True): (118, 0, 19, 1),
-        ('ccx', 3, False, True): (398, 0, 84, 1),
+        ('no_overrides', 1, True, False): (34, 3),
+        ('no_overrides', 2, True, False): (40, 3),
+        ('no_overrides', 3, True, False): (50, 3),
+        ('ccx', 1, True, False): (34, 3),
+        ('ccx', 2, True, False): (40, 3),
+        ('ccx', 3, True, False): (50, 3),
+        ('ccx', 1, True, True): (35, 3),
+        ('ccx', 2, True, True): (41, 3),
+        ('ccx', 3, True, True): (51, 3),
+        ('no_overrides', 1, False, False): (34, 3),
+        ('no_overrides', 2, False, False): (40, 3),
+        ('no_overrides', 3, False, False): (50, 3),
+        ('ccx', 1, False, False): (34, 3),
+        ('ccx', 2, False, False): (40, 3),
+        ('ccx', 3, False, False): (50, 3),
     }

openedx/core/lib/block_structure/block_structure.py

@@ -8,7 +8,6 @@ The following internal data structures are implemented:
     _BlockRelations - Data structure for a single block's relations.
     _BlockData - Data structure for a single block's data.
 """
-from collections import defaultdict
 from logging import getLogger
 
 from openedx.core.lib.graph_traversals import traverse_topologically, traverse_post_order
@@ -58,8 +57,8 @@ class BlockStructure(object):
         # Map of a block's usage key to its block relations. The
         # existence of a block in the structure is determined by its
         # presence in this map.
-        # defaultdict {UsageKey: _BlockRelations}
-        self._block_relations = defaultdict(_BlockRelations)
+        # dict {UsageKey: _BlockRelations}
+        self._block_relations = {}
 
         # Add the root block.
         self._add_block(self._block_relations, root_block_usage_key)
@@ -207,7 +206,7 @@ class BlockStructure(object):
 
         # Create a new block relations map to store only those blocks
         # that are still linked
-        pruned_block_relations = defaultdict(_BlockRelations)
+        pruned_block_relations = {}
         old_block_relations = self._block_relations
 
         # Build the structure from the leaves up by doing a post-order
@@ -245,7 +244,7 @@ class BlockStructure(object):
         relations map.
 
         Arguments:
-            block_relations (defaultdict({UsageKey: _BlockRelations})) -
+            block_relations (dict({UsageKey: _BlockRelations})) -
                 Internal map of a block's usage key to its
                 parents/children relations.
 
@@ -253,6 +252,9 @@ class BlockStructure(object):
 
             child_key (UsageKey) - Usage key of the child block.
         """
+        BlockStructure._add_block(block_relations, parent_key)
+        BlockStructure._add_block(block_relations, child_key)
+
         block_relations[child_key].parents.append(parent_key)
         block_relations[parent_key].children.append(child_key)
 
@@ -262,14 +264,15 @@ class BlockStructure(object):
         Adds the given usage_key to the given block_relations map.
 
         Arguments:
-            block_relations (defaultdict({UsageKey: _BlockRelations})) -
+            block_relations (dict({UsageKey: _BlockRelations})) -
                 Internal map of a block's usage key to its
                 parents/children relations.
 
             usage_key (UsageKey) - Usage key of the block that is to
                 be added to the given block_relations.
         """
-        block_relations[usage_key] = _BlockRelations()
+        if usage_key not in block_relations:
+            block_relations[usage_key] = _BlockRelations()
 
 
 class FieldData(object):