Optimize grading/progress page to reduce database queries (cache max scores).

The progress page did a number of things that make performance terrible for
courses with large numbers of problems, particularly if those problems are
customresponse CapaModule problems that need to be executed via codejail.

The grading code takes pains to not instantiate student state and execute the
problem code. If a student has answered the question, the max score is stored
in StudentModule. However, if the student hasn't attempted the question yet, we
have to run the problem code just to call .max_score() on it. This is necessary
in grade() if the student has answered other problems in the assignment (so we
can know what to divide by). This is always necessary to know in
progress_summary() because we list out every problem there. Code execution can
be especially slow if the problems need to invoke codejail.

To address this, we create a MaxScoresCache that will cache the max raw score
possible for every problem. We select the cache keys so that it will
automatically become invalidated when a new version of the course is published.

The fundamental assumption here is that a problem cannot have two different
max score values for two unscored students. A problem *can* score two students
differently such that they have different max scores. So Carlos can have 2/3 on
a problem, while Lyla gets 3/4. But if neither Carlos nor Lyla has ever
interacted with the problem (i.e. they're just seeing it on their progress
page), they must both see 0/4 -- it cannot be the case that Carlos sees 0/3 and
Lyla sees 0/4.

We used to load all student state into two separate FieldDataCache instances,
after which we do a bunch of individual queries for scored items. Part of this
split-up was done because of locking problems, but I think we might have gotten
overzealous with our manual transaction hammer.

In this commit, we consolidate all state access in grade() and progress()
to use one shared FieldDataCache. We also use a filter so that we only pull
back StudentModule state for things that might possibly affect the grade --
items that either have scores or have children.

Because some older XModules do work in their __init__() methods (like Video),
instantiating them takes time, particularly on large courses. This commit also
changes the code that fetches the grading_context to filter out children that
can't possibly affect the grade.

Finally, we introduce a ScoresClient that also tries to fetch score
information all at once, instead of in separate queries. Technically, we are
fetching this information redundantly, but that's because the state and score
interfaces are being teased apart as we move forward. Still, this only
amounts to one extra SQL query, and has very little impact on performance
overall.

Much thanks to @adampalay -- his hackathon work in #7168 formed the basis of
this.

https://openedx.atlassian.net/browse/CSM-17

common/lib/xmodule/xmodule/course_module.py

@@ -20,6 +20,7 @@ from xmodule.tabs import CourseTabList
 from xmodule.mixin import LicenseMixin
 import json
 
+from xblock.core import XBlock
 from xblock.fields import Scope, List, String, Dict, Boolean, Integer, Float
 from .fields import Date
 from django.utils.timezone import UTC
@@ -1321,11 +1322,15 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
         except UndefinedContext:
             module = self
 
+        def possibly_scored(usage_key):
+            """Can this XBlock type can have a score or children?"""
+            return usage_key.block_type in self.block_types_affecting_grading
+
         all_descriptors = []
         graded_sections = {}
 
         def yield_descriptor_descendents(module_descriptor):
-            for child in module_descriptor.get_children():
+            for child in module_descriptor.get_children(usage_key_filter=possibly_scored):
                 yield child
                 for module_descriptor in yield_descriptor_descendents(child):
                     yield module_descriptor
@@ -1351,6 +1356,15 @@ class CourseDescriptor(CourseFields, SequenceDescriptor, LicenseMixin):
         return {'graded_sections': graded_sections,
                 'all_descriptors': all_descriptors, }
 
+    @lazy
+    def block_types_affecting_grading(self):
+        """Return all block types that could impact grading (i.e. scored, or having children)."""
+        return frozenset(
+            cat for (cat, xblock_class) in XBlock.load_classes() if (
+                getattr(xblock_class, 'has_score', False) or getattr(xblock_class, 'has_children', False)
+            )
+        )
+
     @staticmethod
     def make_id(org, course, url_name):
         return '/'.join([org, course, url_name])

lms/djangoapps/ccx/tests/test_field_override_performance.py

@@ -29,7 +29,11 @@ from xmodule.modulestore.tests.utils import ProceduralCourseTestMixin
 
 @attr('shard_1')
 @mock.patch.dict(
-    'django.conf.settings.FEATURES', {'ENABLE_XBLOCK_VIEW_ENDPOINT': True}
+    'django.conf.settings.FEATURES',
+    {
+        'ENABLE_XBLOCK_VIEW_ENDPOINT': True,
+        'ENABLE_MAX_SCORE_CACHE': False,
+    }
 )
 @ddt.ddt
 class FieldOverridePerformanceTestCase(ProceduralCourseTestMixin,
@@ -173,18 +177,18 @@ class TestFieldOverrideMongoPerformance(FieldOverridePerformanceTestCase):
 
     TEST_DATA = {
         # (providers, course_width, enable_ccx): # of sql queries, # of mongo queries, # of xblocks
-        ('no_overrides', 1, True): (27, 7, 14),
-        ('no_overrides', 2, True): (135, 7, 85),
-        ('no_overrides', 3, True): (595, 7, 336),
-        ('ccx', 1, True): (27, 7, 14),
-        ('ccx', 2, True): (135, 7, 85),
-        ('ccx', 3, True): (595, 7, 336),
-        ('no_overrides', 1, False): (27, 7, 14),
-        ('no_overrides', 2, False): (135, 7, 85),
-        ('no_overrides', 3, False): (595, 7, 336),
-        ('ccx', 1, False): (27, 7, 14),
-        ('ccx', 2, False): (135, 7, 85),
-        ('ccx', 3, False): (595, 7, 336),
+        ('no_overrides', 1, True): (23, 7, 14),
+        ('no_overrides', 2, True): (68, 7, 85),
+        ('no_overrides', 3, True): (263, 7, 336),
+        ('ccx', 1, True): (23, 7, 14),
+        ('ccx', 2, True): (68, 7, 85),
+        ('ccx', 3, True): (263, 7, 336),
+        ('no_overrides', 1, False): (23, 7, 14),
+        ('no_overrides', 2, False): (68, 7, 85),
+        ('no_overrides', 3, False): (263, 7, 336),
+        ('ccx', 1, False): (23, 7, 14),
+        ('ccx', 2, False): (68, 7, 85),
+        ('ccx', 3, False): (263, 7, 336),
     }
 
 
@@ -196,16 +200,16 @@ class TestFieldOverrideSplitPerformance(FieldOverridePerformanceTestCase):
     __test__ = True
 
     TEST_DATA = {
-        ('no_overrides', 1, True): (27, 4, 9),
-        ('no_overrides', 2, True): (135, 19, 54),
-        ('no_overrides', 3, True): (595, 84, 215),
-        ('ccx', 1, True): (27, 4, 9),
-        ('ccx', 2, True): (135, 19, 54),
-        ('ccx', 3, True): (595, 84, 215),
-        ('no_overrides', 1, False): (27, 4, 9),
-        ('no_overrides', 2, False): (135, 19, 54),
-        ('no_overrides', 3, False): (595, 84, 215),
-        ('ccx', 1, False): (27, 4, 9),
-        ('ccx', 2, False): (135, 19, 54),
-        ('ccx', 3, False): (595, 84, 215),
+        ('no_overrides', 1, True): (23, 4, 9),
+        ('no_overrides', 2, True): (68, 19, 54),
+        ('no_overrides', 3, True): (263, 84, 215),
+        ('ccx', 1, True): (23, 4, 9),
+        ('ccx', 2, True): (68, 19, 54),
+        ('ccx', 3, True): (263, 84, 215),
+        ('no_overrides', 1, False): (23, 4, 9),
+        ('no_overrides', 2, False): (68, 19, 54),
+        ('no_overrides', 3, False): (263, 84, 215),
+        ('ccx', 1, False): (23, 4, 9),
+        ('ccx', 2, False): (68, 19, 54),
+        ('ccx', 3, False): (263, 84, 215),
     }

lms/djangoapps/courseware/model_data.py

@@ -23,7 +23,7 @@ DjangoOrmFieldCache: A base-class for single-row-per-field caches.
 
 import json
 from abc import abstractmethod, ABCMeta
-from collections import defaultdict
+from collections import defaultdict, namedtuple
 from .models import (
     StudentModule,
     XModuleUserStateSummaryField,
@@ -741,6 +741,7 @@ class FieldDataCache(object):
                 self.course_id,
             ),
         }
+        self.scorable_locations = set()
         self.add_descriptors_to_cache(descriptors)
 
     def add_descriptors_to_cache(self, descriptors):
@@ -748,6 +749,7 @@ class FieldDataCache(object):
         Add all `descriptors` to this FieldDataCache.
         """
         if self.user.is_authenticated():
+            self.scorable_locations.update(desc.location for desc in descriptors if desc.has_score)
             for scope, fields in self._fields_to_cache(descriptors).items():
                 if scope not in self.cache:
                     continue
@@ -955,3 +957,63 @@ class FieldDataCache(object):
 
     def __len__(self):
         return sum(len(cache) for cache in self.cache.values())
+
+
+class ScoresClient(object):
+    """
+    Basic client interface for retrieving Score information.
+
+    Eventually, this should read and write scores, but at the moment it only
+    handles the read side of things.
+    """
+    Score = namedtuple('Score', 'correct total')
+
+    def __init__(self, course_key, user_id):
+        """Basic constructor. from_field_data_cache() is more appopriate for most uses."""
+        self.course_key = course_key
+        self.user_id = user_id
+        self._locations_to_scores = {}
+        self._has_fetched = False
+
+    def __contains__(self, location):
+        """Return True if we have a score for this location."""
+        return location in self._locations_to_scores
+
+    def fetch_scores(self, locations):
+        """Grab score information."""
+        scores_qset = StudentModule.objects.filter(
+            student_id=self.user_id,
+            course_id=self.course_key,
+            module_state_key__in=set(locations),
+        )
+        # Locations in StudentModule don't necessarily have course key info
+        # attached to them (since old mongo identifiers don't include runs).
+        # So we have to add that info back in before we put it into our lookup.
+        self._locations_to_scores.update({
+            UsageKey.from_string(location).map_into_course(self.course_key): self.Score(correct, total)
+            for location, correct, total
+            in scores_qset.values_list('module_state_key', 'grade', 'max_grade')
+        })
+        self._has_fetched = True
+
+    def get(self, location):
+        """
+        Get the score for a given location, if it exists.
+
+        If we don't have a score for that location, return `None`. Note that as
+        convention, you should be passing in a location with full course run
+        information.
+        """
+        if not self._has_fetched:
+            raise ValueError(
+                "Tried to fetch location {} from ScoresClient before fetch_scores() has run."
+                .format(location)
+            )
+        return self._locations_to_scores.get(location)
+
+    @classmethod
+    def from_field_data_cache(cls, fd_cache):
+        """Create a ScoresClient from a populated FieldDataCache."""
+        client = cls(fd_cache.course_id, fd_cache.user.id)
+        client.fetch_scores(fd_cache.scorable_locations)
+        return client

lms/djangoapps/courseware/models.py

@@ -133,7 +133,10 @@ class StudentModule(models.Model):
         return 'StudentModule<%r>' % ({
             'course_id': self.course_id,
             'module_type': self.module_type,
-            'student': self.student.username,  # pylint: disable=no-member
+            # We use the student_id instead of username to avoid a database hop.
+            # This can actually matter in cases where we're logging many of
+            # these (e.g. on a broken progress page).
+            'student_id': self.student_id,  # pylint: disable=no-member
             'module_state_key': self.module_state_key,
             'state': str(self.state)[:20],
         },)

lms/djangoapps/courseware/tests/test_grades.py

@@ -2,13 +2,16 @@
 Test grade calculation.
 """
 from django.http import Http404
+from django.test.client import RequestFactory
+
 from mock import patch
 from nose.plugins.attrib import attr
 from opaque_keys.edx.locations import SlashSeparatedCourseKey
 
-from courseware.grades import grade, iterate_grades_for
+from courseware.grades import field_data_cache_for_grading, grade, iterate_grades_for, MaxScoresCache
 from student.tests.factories import UserFactory
-from xmodule.modulestore.tests.factories import CourseFactory
+from student.models import CourseEnrollment
+from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory
 from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 
@@ -121,3 +124,73 @@ class TestGradeIteration(ModuleStoreTestCase):
                 students_to_errors[student] = err_msg
 
         return students_to_gradesets, students_to_errors
+
+
+class TestMaxScoresCache(ModuleStoreTestCase):
+    """
+    Tests for the MaxScoresCache
+    """
+    def setUp(self):
+        super(TestMaxScoresCache, self).setUp()
+        self.student = UserFactory.create()
+        self.course = CourseFactory.create()
+        self.problems = []
+        for _ in xrange(3):
+            self.problems.append(
+                ItemFactory.create(category='problem', parent=self.course)
+            )
+
+        CourseEnrollment.enroll(self.student, self.course.id)
+        self.request = RequestFactory().get('/')
+        self.locations = [problem.location for problem in self.problems]
+
+    def test_max_scores_cache(self):
+        """
+        Tests the behavior fo the MaxScoresCache
+        """
+        max_scores_cache = MaxScoresCache("test_max_scores_cache")
+        self.assertEqual(max_scores_cache.num_cached_from_remote(), 0)
+        self.assertEqual(max_scores_cache.num_cached_updates(), 0)
+
+        # add score to cache
+        max_scores_cache.set(self.locations[0], 1)
+        self.assertEqual(max_scores_cache.num_cached_updates(), 1)
+
+        # push to remote cache
+        max_scores_cache.push_to_remote()
+
+        # create a new cache with the same params, fetch from remote cache
+        max_scores_cache = MaxScoresCache("test_max_scores_cache")
+        max_scores_cache.fetch_from_remote(self.locations)
+
+        # see cache is populated
+        self.assertEqual(max_scores_cache.num_cached_from_remote(), 1)
+
+
+class TestFieldDataCacheScorableLocations(ModuleStoreTestCase):
+    """
+    Make sure we can filter the locations we pull back student state for via
+    the FieldDataCache.
+    """
+    def setUp(self):
+        super(TestFieldDataCacheScorableLocations, self).setUp()
+        self.student = UserFactory.create()
+        self.course = CourseFactory.create()
+        chapter = ItemFactory.create(category='chapter', parent=self.course)
+        sequential = ItemFactory.create(category='sequential', parent=chapter)
+        vertical = ItemFactory.create(category='vertical', parent=sequential)
+        ItemFactory.create(category='video', parent=vertical)
+        ItemFactory.create(category='html', parent=vertical)
+        ItemFactory.create(category='discussion', parent=vertical)
+        ItemFactory.create(category='problem', parent=vertical)
+
+        CourseEnrollment.enroll(self.student, self.course.id)
+
+    def test_field_data_cache_scorable_locations(self):
+        """Only scorable locations should be in FieldDataCache.scorable_locations."""
+        fd_cache = field_data_cache_for_grading(self.course, self.student)
+        block_types = set(loc.block_type for loc in fd_cache.scorable_locations)
+        self.assertNotIn('video', block_types)
+        self.assertNotIn('html', block_types)
+        self.assertNotIn('discussion', block_types)
+        self.assertIn('problem', block_types)

lms/djangoapps/courseware/tests/test_model_data.py

@@ -6,8 +6,7 @@ from mock import Mock, patch
 from nose.plugins.attrib import attr
 from functools import partial
 
-from courseware.model_data import DjangoKeyValueStore
-from courseware.model_data import InvalidScopeError, FieldDataCache
+from courseware.model_data import DjangoKeyValueStore, FieldDataCache, InvalidScopeError
 from courseware.models import StudentModule
 from courseware.models import XModuleStudentInfoField, XModuleStudentPrefsField
 

lms/djangoapps/courseware/tests/test_submitting_problems.py

@@ -109,6 +109,15 @@ class TestSubmittingProblems(ModuleStoreTestCase, LoginEnrollmentTestCase):
 
         return resp
 
+    def look_at_question(self, problem_url_name):
+        """
+        Create state for a problem, but don't answer it
+        """
+        location = self.problem_location(problem_url_name)
+        modx_url = self.modx_url(location, "problem_get")
+        resp = self.client.get(modx_url)
+        return resp
+
     def reset_question_answer(self, problem_url_name):
         """
         Reset specified problem for current user.
@@ -457,6 +466,33 @@ class TestCourseGrader(TestSubmittingProblems):
         current_count = csmh.count()
         self.assertEqual(current_count, 3)
 
+    def test_grade_with_max_score_cache(self):
+        """
+        Tests that the max score cache is populated after a grading run
+        and that the results of grading runs before and after the cache
+        warms are the same.
+        """
+        self.basic_setup()
+        self.submit_question_answer('p1', {'2_1': 'Correct'})
+        self.look_at_question('p2')
+        self.assertTrue(
+            StudentModule.objects.filter(
+                module_state_key=self.problem_location('p2')
+            ).exists()
+        )
+        location_to_cache = unicode(self.problem_location('p2'))
+        max_scores_cache = grades.MaxScoresCache.create_for_course(self.course)
+
+        # problem isn't in the cache
+        max_scores_cache.fetch_from_remote([location_to_cache])
+        self.assertIsNone(max_scores_cache.get(location_to_cache))
+        self.check_grade_percent(0.33)
+
+        # problem is in the cache
+        max_scores_cache.fetch_from_remote([location_to_cache])
+        self.assertIsNotNone(max_scores_cache.get(location_to_cache))
+        self.check_grade_percent(0.33)
+
     def test_none_grade(self):
         """
         Check grade is 0 to begin with.
@@ -474,6 +510,13 @@ class TestCourseGrader(TestSubmittingProblems):
         self.check_grade_percent(0.33)
         self.assertEqual(self.get_grade_summary()['grade'], 'B')
 
+    @patch.dict("django.conf.settings.FEATURES", {"ENABLE_MAX_SCORE_CACHE": False})
+    def test_grade_no_max_score_cache(self):
+        """
+        Tests grading when the max score cache is disabled
+        """
+        self.test_b_grade_exact()
+
     def test_b_grade_above(self):
         """
         Check grade between cutoffs.

lms/djangoapps/courseware/views.py

@@ -44,7 +44,7 @@ from openedx.core.djangoapps.credit.api import (
     is_user_eligible_for_credit,
     is_credit_course
 )
-from courseware.model_data import FieldDataCache
+from courseware.model_data import FieldDataCache, ScoresClient
 from .module_render import toc_for_course, get_module_for_descriptor, get_module, get_module_by_usage_id
 from .entrance_exams import (
     course_has_entrance_exam,
@@ -1054,10 +1054,15 @@ def _progress(request, course_key, student_id):
     # The pre-fetching of groups is done to make auth checks not require an
     # additional DB lookup (this kills the Progress page in particular).
     student = User.objects.prefetch_related("groups").get(id=student.id)
-
-    courseware_summary = grades.progress_summary(student, request, course)
+    field_data_cache = grades.field_data_cache_for_grading(course, student)
+    scores_client = ScoresClient.from_field_data_cache(field_data_cache)
+    courseware_summary = grades.progress_summary(
+        student, request, course, field_data_cache=field_data_cache, scores_client=scores_client
+    )
+    grade_summary = grades.grade(
+        student, request, course, field_data_cache=field_data_cache, scores_client=scores_client
+    )
     studio_url = get_studio_url(course, 'settings/grading')
-    grade_summary = grades.grade(student, request, course)
 
     if courseware_summary is None:
         #This means the student didn't have access to the course (which the instructor requested)

lms/djangoapps/instructor_task/tests/test_tasks_helper.py

@@ -1336,7 +1336,7 @@ class TestCertificateGeneration(InstructorTaskModuleTestCase):
 
         current_task = Mock()
         current_task.update_state = Mock()
-        with self.assertNumQueries(109):
+        with self.assertNumQueries(125):
             with patch('instructor_task.tasks_helper._get_current_task') as mock_current_task:
                 mock_current_task.return_value = current_task
                 with patch('capa.xqueue_interface.XQueueInterface.send_to_queue') as mock_queue:

lms/envs/common.py

@@ -40,6 +40,7 @@ from django.utils.translation import ugettext_lazy as _
 from .discussionsettings import *
 import dealer.git
 from xmodule.modulestore.modulestore_settings import update_module_store_settings
+from xmodule.modulestore.edit_info import EditInfoMixin
 from xmodule.mixin import LicenseMixin
 from lms.djangoapps.lms_xblock.mixin import LmsBlockMixin
 
@@ -416,6 +417,9 @@ FEATURES = {
 
     # The block types to disable need to be specified in "x block disable config" in django admin.
     'ENABLE_DISABLING_XBLOCK_TYPES': True,
+
+    # Enable the max score cache to speed up grading
+    'ENABLE_MAX_SCORE_CACHE': True,
 }
 
 # Ignore static asset files on import which match this pattern
@@ -711,7 +715,7 @@ from xmodule.x_module import XModuleMixin
 # These are the Mixins that should be added to every XBlock.
 # This should be moved into an XBlock Runtime/Application object
 # once the responsibility of XBlock creation is moved out of modulestore - cpennington
-XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin)
+XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin, EditInfoMixin)
 
 # Allow any XBlock in the LMS
 XBLOCK_SELECT_FUNCTION = prefer_xmodules