From e624ff3b04bde9f4afb96bd474e33d7bb00090e0 Mon Sep 17 00:00:00 2001 From: Don Mitchell Date: Fri, 22 Aug 2014 16:34:20 -0400 Subject: [PATCH] Make path_to_location use bulk_write_operation for performance. --- .../lib/xmodule/xmodule/modulestore/mixed.py | 5 +- .../lib/xmodule/xmodule/modulestore/search.py | 66 ++++++++++--------- .../tests/test_mixed_modulestore.py | 3 +- lms/djangoapps/courseware/model_data.py | 4 +- .../courseware/tests/test_module_render.py | 9 +-- 5 files changed, 47 insertions(+), 40 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mixed.py b/common/lib/xmodule/xmodule/modulestore/mixed.py index 4092306e9e..47398a5ed8 100644 --- a/common/lib/xmodule/xmodule/modulestore/mixed.py +++ b/common/lib/xmodule/xmodule/modulestore/mixed.py @@ -651,5 +651,8 @@ class MixedModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase): If course_id is None, the default store is used. """ store = self._get_modulestore_for_courseid(course_id) - with store.bulk_write_operations(course_id): + if hasattr(store, 'bulk_write_operations'): + with store.bulk_write_operations(course_id): + yield + else: yield diff --git a/common/lib/xmodule/xmodule/modulestore/search.py b/common/lib/xmodule/xmodule/modulestore/search.py index 161e6e2cef..901bcf85f5 100644 --- a/common/lib/xmodule/xmodule/modulestore/search.py +++ b/common/lib/xmodule/xmodule/modulestore/search.py @@ -68,39 +68,41 @@ def path_to_location(modulestore, usage_key): newpath = (next_usage, path) queue.append((parent, newpath)) - if not modulestore.has_item(usage_key): - raise ItemNotFoundError(usage_key) + # doesn't write but does multiple reads. bulk_write minimizes reads too + with modulestore.bulk_write_operations(usage_key.course_key): + if not modulestore.has_item(usage_key): + raise ItemNotFoundError(usage_key) - path = find_path_to_course() - if path is None: - raise NoPathToItem(usage_key) + path = find_path_to_course() + if path is None: + raise NoPathToItem(usage_key) - n = len(path) - course_id = path[0].course_key - # pull out the location names - chapter = path[1].name if n > 1 else None - section = path[2].name if n > 2 else None - # Figure out the position - position = None + n = len(path) + course_id = path[0].course_key + # pull out the location names + chapter = path[1].name if n > 1 else None + section = path[2].name if n > 2 else None + # Figure out the position + position = None - # This block of code will find the position of a module within a nested tree - # of modules. If a problem is on tab 2 of a sequence that's on tab 3 of a - # sequence, the resulting position is 3_2. However, no positional modules - # (e.g. sequential and videosequence) currently deal with this form of - # representing nested positions. This needs to happen before jumping to a - # module nested in more than one positional module will work. - if n > 3: - position_list = [] - for path_index in range(2, n - 1): - category = path[path_index].block_type - if category == 'sequential' or category == 'videosequence': - section_desc = modulestore.get_item(path[path_index]) - # this calls get_children rather than just children b/c old mongo includes private children - # in children but not in get_children - child_locs = [c.location for c in section_desc.get_children()] - # positions are 1-indexed, and should be strings to be consistent with - # url parsing. - position_list.append(str(child_locs.index(path[path_index + 1]) + 1)) - position = "_".join(position_list) + # This block of code will find the position of a module within a nested tree + # of modules. If a problem is on tab 2 of a sequence that's on tab 3 of a + # sequence, the resulting position is 3_2. However, no positional modules + # (e.g. sequential and videosequence) currently deal with this form of + # representing nested positions. This needs to happen before jumping to a + # module nested in more than one positional module will work. + if n > 3: + position_list = [] + for path_index in range(2, n - 1): + category = path[path_index].block_type + if category == 'sequential' or category == 'videosequence': + section_desc = modulestore.get_item(path[path_index]) + # this calls get_children rather than just children b/c old mongo includes private children + # in children but not in get_children + child_locs = [c.location for c in section_desc.get_children()] + # positions are 1-indexed, and should be strings to be consistent with + # url parsing. + position_list.append(str(child_locs.index(path[path_index + 1]) + 1)) + position = "_".join(position_list) - return (course_id, chapter, section, position) + return (course_id, chapter, section, position) diff --git a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py index 1f8dea5485..4091e5adf4 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -717,7 +717,7 @@ class TestMixedModuleStore(unittest.TestCase): # TODO: LMS-11220: Document why draft send count is 5 # TODO: LMS-11220: Document why draft find count is 18 # TODO: LMS-11220: Document why split find count is 16 - @ddt.data(('draft', [18, 5], 0), ('split', [16, 6], 0)) + @ddt.data(('draft', [19, 5], 0), ('split', [2, 2], 0)) @ddt.unpack def test_path_to_location(self, default_ms, num_finds, num_sends): """ @@ -736,7 +736,6 @@ class TestMixedModuleStore(unittest.TestCase): (course_key, "Chapter_x", None, None)), ) - mongo_store = self.store._get_modulestore_for_courseid(self._course_key_from_string(self.MONGO_COURSEID)) for location, expected in should_work: with check_mongo_calls(num_finds.pop(0), num_sends): self.assertEqual(path_to_location(self.store, location), expected) diff --git a/lms/djangoapps/courseware/model_data.py b/lms/djangoapps/courseware/model_data.py index da45edf731..cb3097a967 100644 --- a/lms/djangoapps/courseware/model_data.py +++ b/lms/djangoapps/courseware/model_data.py @@ -21,6 +21,7 @@ from django.contrib.auth.models import User from xblock.runtime import KeyValueStore from xblock.exceptions import KeyValueMultiSaveError, InvalidScopeError from xblock.fields import Scope, UserScope +from xmodule.modulestore.django import modulestore log = logging.getLogger(__name__) @@ -109,7 +110,8 @@ class FieldDataCache(object): return descriptors - descriptors = get_child_descriptors(descriptor, depth, descriptor_filter) + with modulestore().bulk_write_operations(descriptor.location.course_key): + descriptors = get_child_descriptors(descriptor, depth, descriptor_filter) return FieldDataCache(descriptors, course_id, user, select_for_update) diff --git a/lms/djangoapps/courseware/tests/test_module_render.py b/lms/djangoapps/courseware/tests/test_module_render.py index 4769a3373c..e14a51b8ff 100644 --- a/lms/djangoapps/courseware/tests/test_module_render.py +++ b/lms/djangoapps/courseware/tests/test_module_render.py @@ -326,14 +326,15 @@ class TestTOC(ModuleStoreTestCase): self.request = factory.get(chapter_url) self.request.user = UserFactory() self.modulestore = self.store._get_modulestore_for_courseid(self.course_key) + self.toy_course = self.store.get_course(self.toy_loc, depth=2) with check_mongo_calls(num_finds, num_sends): - self.toy_course = self.store.get_course(self.toy_loc, depth=2) self.field_data_cache = FieldDataCache.cache_for_descriptor_descendents( - self.toy_loc, self.request.user, self.toy_course, depth=2) + self.toy_loc, self.request.user, self.toy_course, depth=2 + ) # TODO: LMS-11220: Document why split find count is 21 - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 21, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 5, 0)) @ddt.unpack def test_toc_toy_from_chapter(self, default_ms, num_finds, num_sends): with self.store.default_store(default_ms): @@ -361,7 +362,7 @@ class TestTOC(ModuleStoreTestCase): self.assertIn(toc_section, actual) # TODO: LMS-11220: Document why split find count is 21 - @ddt.data((ModuleStoreEnum.Type.mongo, 3, 0), (ModuleStoreEnum.Type.split, 21, 0)) + @ddt.data((ModuleStoreEnum.Type.mongo, 1, 0), (ModuleStoreEnum.Type.split, 5, 0)) @ddt.unpack def test_toc_toy_from_section(self, default_ms, num_finds, num_sends): with self.store.default_store(default_ms):