From 86eca26aa0d4e28911b2760c00b4fed0a05a470a Mon Sep 17 00:00:00 2001 From: Sanford Student Date: Wed, 28 Jun 2017 12:15:27 -0400 Subject: [PATCH] EDUCATOR-778: prefetched visible blocks --- lms/djangoapps/grades/models.py | 60 ++++++++++++++++--- .../grades/new/course_grade_factory.py | 54 ++++++++++------- lms/djangoapps/grades/tests/test_tasks.py | 11 ---- 3 files changed, 84 insertions(+), 41 deletions(-) diff --git a/lms/djangoapps/grades/models.py b/lms/djangoapps/grades/models.py index b4d0c2c01e..ea9a7d27f7 100644 --- a/lms/djangoapps/grades/models.py +++ b/lms/djangoapps/grades/models.py @@ -181,6 +181,7 @@ class VisibleBlocks(models.Model): in the blocks_json field. A hash of this json array is used for lookup purposes. """ + CACHE_NAMESPACE = u"grades.models.VisibleBlocks" blocks_json = models.TextField() hashed = models.CharField(max_length=100, unique=True) course_id = CourseKeyField(blank=False, max_length=255, db_index=True) @@ -207,27 +208,37 @@ class VisibleBlocks(models.Model): @classmethod def bulk_read(cls, course_key): """ - Reads all visible block records for the given course. + Reads and returns all visible block records for the given course from + the cache. The cache is initialize with the visible blocks for this + course if no entry currently exists.has no entry for this course, + the cache is updated. Arguments: course_key: The course identifier for the desired records """ - return cls.objects.filter(course_id=course_key) + prefetched = get_cache(cls.CACHE_NAMESPACE).get(cls._cache_key(course_key)) + if not prefetched: + prefetched = cls._initialize_cache(course_key) + return prefetched @classmethod - def bulk_create(cls, block_record_lists): + def bulk_create(cls, course_key, block_record_lists): """ Bulk creates VisibleBlocks for the given iterator of - BlockRecordList objects. + BlockRecordList objects and updates the VisibleBlocks cache + for the block records' course with the new VisibleBlocks. + Returns the newly created visible blocks. """ - return cls.objects.bulk_create([ + created = cls.objects.bulk_create([ VisibleBlocks( blocks_json=brl.json_value, hashed=brl.hash_value, - course_id=brl.course_key, + course_id=course_key, ) for brl in block_record_lists ]) + cls._update_cache(course_key, created) + return created @classmethod def bulk_get_or_create(cls, block_record_lists, course_key): @@ -236,9 +247,42 @@ class VisibleBlocks(models.Model): BlockRecordList objects for the given course_key, but only for those that aren't already created. """ - existent_records = {record.hashed: record for record in cls.bulk_read(course_key)} + existent_records = cls.bulk_read(course_key) non_existent_brls = {brl for brl in block_record_lists if brl.hash_value not in existent_records} - cls.bulk_create(non_existent_brls) + cls.bulk_create(course_key, non_existent_brls) + + @classmethod + def _initialize_cache(cls, course_key): + """ + Prefetches visible blocks for the given course and stores in the cache. + Returns a dictionary mapping hashes of these block records to the + block record objects. + """ + prefetched = {record.hashed: record for record in cls.objects.filter(course_id=course_key)} + get_cache(cls.CACHE_NAMESPACE)[cls._cache_key(course_key)] = prefetched + return prefetched + + @classmethod + def _update_cache(cls, course_key, visible_blocks): + """ + Adds a specific set of visible blocks to the request cache. + This assumes that prefetch has already been called. + """ + get_cache(cls.CACHE_NAMESPACE)[cls._cache_key(course_key)].update( + {visible_block.hashed: visible_block for visible_block in visible_blocks} + ) + + @classmethod + def clear_cache(cls, course_key): + """ + Clears the cache of all contents for a given course. + """ + cache = get_cache(cls.CACHE_NAMESPACE) + cache.pop(cls._cache_key(course_key), None) + + @classmethod + def _cache_key(cls, course_key): + return u"visible_blocks_cache.{}".format(course_key) class PersistentSubsectionGrade(DeleteGradesMixin, TimeStampedModel): diff --git a/lms/djangoapps/grades/new/course_grade_factory.py b/lms/djangoapps/grades/new/course_grade_factory.py index 56b4eca040..0f82ceca0f 100644 --- a/lms/djangoapps/grades/new/course_grade_factory.py +++ b/lms/djangoapps/grades/new/course_grade_factory.py @@ -1,4 +1,5 @@ from collections import namedtuple +from contextlib import contextmanager from logging import getLogger import dogstats_wrapper as dog_stats_api @@ -6,7 +7,7 @@ from openedx.core.djangoapps.signals.signals import COURSE_GRADE_CHANGED from ..config import assume_zero_if_absent, should_persist_grades from ..config.waffle import WRITE_ONLY_IF_ENGAGED, waffle -from ..models import PersistentCourseGrade +from ..models import PersistentCourseGrade, VisibleBlocks from .course_data import CourseData from .course_grade import CourseGrade, ZeroCourseGrade @@ -75,6 +76,14 @@ class CourseGradeFactory(object): course_data = CourseData(user, course, collected_block_structure, course_structure, course_key) return self._update(user, course_data, read_only=False) + @contextmanager + def _course_transaction(self, course_key): + """ + Provides a transaction context in which GradeResults are created. + """ + yield + VisibleBlocks.clear_cache(course_key) + def iter( self, users, @@ -100,28 +109,29 @@ class CourseGradeFactory(object): course_data = CourseData( user=None, course=course, collected_block_structure=collected_block_structure, course_key=course_key, ) - for user in users: - with dog_stats_api.timer( - 'lms.grades.CourseGradeFactory.iter', - tags=[u'action:{}'.format(course_data.course_key)] - ): - try: - method = CourseGradeFactory().update if force_update else CourseGradeFactory().create - course_grade = method( - user, course_data.course, course_data.collected_structure, course_key=course_key, - ) - yield self.GradeResult(user, course_grade, None) + stats_tags = [u'action:{}'.format(course_data.course_key)] + with self._course_transaction(course_data.course_key): + for user in users: + with dog_stats_api.timer('lms.grades.CourseGradeFactory.iter', tags=stats_tags): + yield self._iter_grade_result(user, course_data, force_update) - except Exception as exc: # pylint: disable=broad-except - # Keep marching on even if this student couldn't be graded for - # some reason, but log it for future reference. - log.exception( - 'Cannot grade student %s in course %s because of exception: %s', - user.id, - course_data.course_key, - exc.message - ) - yield self.GradeResult(user, None, exc) + def _iter_grade_result(self, user, course_data, force_update): + try: + method = CourseGradeFactory().update if force_update else CourseGradeFactory().create + course_grade = method( + user, course_data.course, course_data.collected_structure, course_key=course_data.course_key, + ) + return self.GradeResult(user, course_grade, None) + except Exception as exc: # pylint: disable=broad-except + # Keep marching on even if this student couldn't be graded for + # some reason, but log it for future reference. + log.exception( + 'Cannot grade student %s in course %s because of exception: %s', + user.id, + course_data.course_key, + exc.message + ) + return self.GradeResult(user, None, exc) @staticmethod def _create_zero(user, course_data): diff --git a/lms/djangoapps/grades/tests/test_tasks.py b/lms/djangoapps/grades/tests/test_tasks.py index 5a60c2c69d..39556b77d2 100644 --- a/lms/djangoapps/grades/tests/test_tasks.py +++ b/lms/djangoapps/grades/tests/test_tasks.py @@ -406,17 +406,6 @@ class ComputeGradesForCourseTest(HasCourseWithProblemsMixin, ModuleStoreTestCase min(batch_size, 8) # No more than 8 due to offset ) - @ddt.data(*xrange(1, 12, 3)) - def test_database_calls(self, batch_size): - per_user_queries = 15 * min(batch_size, 6) # No more than 6 due to offset - with self.assertNumQueries(6 + per_user_queries): - with check_mongo_calls(1): - compute_grades_for_course_v2.delay( - course_key=six.text_type(self.course.id), - batch_size=batch_size, - offset=6, - ) - @ddt.data(*xrange(1, 12, 3)) def test_compute_all_grades_for_course(self, batch_size): self.set_up_course()