From 352e2190eb7a593b781adf20cb4f5080415e1337 Mon Sep 17 00:00:00 2001 From: Muhammad Rehan Date: Wed, 30 Dec 2015 16:42:00 +0500 Subject: [PATCH] improve get_items and has_path_to_root with temporary caches. --- .../xmodule/xmodule/modulestore/mongo/base.py | 8 +-- .../xmodule/modulestore/split_mongo/split.py | 61 +++++++++++++++++-- .../xmodule/modulestore/store_utilities.py | 3 + .../tests/test_mixed_modulestore.py | 8 +-- 4 files changed, 64 insertions(+), 16 deletions(-) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo/base.py b/common/lib/xmodule/xmodule/modulestore/mongo/base.py index 840fd48c03..45b5d6de5c 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo/base.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo/base.py @@ -51,6 +51,7 @@ from xmodule.modulestore.edit_info import EditInfoRuntimeMixin from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError from xmodule.modulestore.inheritance import InheritanceMixin, inherit_metadata, InheritanceKeyValueStore from xmodule.modulestore.xml import CourseLocationManager +from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES from xmodule.services import SettingsService log = logging.getLogger(__name__) @@ -77,8 +78,6 @@ BLOCK_TYPES_WITH_CHILDREN = list(set( # at module level, cache one instance of OSFS per filesystem root. _OSFS_INSTANCE = {} -_DETACHED_CATEGORIES = [name for name, __ in XBlock.load_tagged_classes("detached")] - class MongoRevisionKey(object): """ @@ -269,7 +268,8 @@ class CachingDescriptorSystem(MakoDescriptorSystem, EditInfoRuntimeMixin): ) if parent_url: parent = self._convert_reference_to_key(parent_url) - if not parent and category not in _DETACHED_CATEGORIES + ['course']: + + if not parent and category not in DETACHED_XBLOCK_TYPES.union(['course']): # try looking it up just-in-time (but not if we're working with a detached block). parent = self.modulestore.get_parent_location( as_published(location), @@ -965,7 +965,7 @@ class MongoModuleStore(ModuleStoreDraftAndPublished, ModuleStoreWriteBase, Mongo of inherited metadata onto the item """ category = item['location']['category'] - apply_cached_metadata = category not in _DETACHED_CATEGORIES and \ + apply_cached_metadata = category not in DETACHED_XBLOCK_TYPES and \ not (category == 'course' and depth == 0) return apply_cached_metadata diff --git a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py index 883dd7609d..d90eb594e7 100644 --- a/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py +++ b/common/lib/xmodule/xmodule/modulestore/split_mongo/split.py @@ -83,6 +83,7 @@ from ..exceptions import ItemNotFoundError from .caching_descriptor_system import CachingDescriptorSystem from xmodule.modulestore.split_mongo.mongo_connection import MongoConnection, DuplicateKeyError from xmodule.modulestore.split_mongo import BlockKey, CourseEnvelope +from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES from xmodule.error_module import ErrorDescriptor from collections import defaultdict from types import NoneType @@ -1197,15 +1198,25 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): if 'category' in qualifiers: qualifiers['block_type'] = qualifiers.pop('category') - detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")] - # don't expect caller to know that children are in fields if 'children' in qualifiers: settings['children'] = qualifiers.pop('children') + + # No need of these caches unless include_orphans is set to False + path_cache = None + parents_cache = None + + if not include_orphans: + path_cache = {} + parents_cache = self.build_block_key_to_parents_mapping(course.structure) + for block_id, value in course.structure['blocks'].iteritems(): if _block_matches_all(value): if not include_orphans: - if self.has_path_to_root(block_id, course) or block_id.type in detached_categories: + if ( # pylint: disable=bad-continuation + block_id.type in DETACHED_XBLOCK_TYPES or + self.has_path_to_root(block_id, course, path_cache, parents_cache) + ): items.append(block_id) else: items.append(block_id) @@ -1215,22 +1226,60 @@ class SplitMongoModuleStore(SplitBulkWriteMixin, ModuleStoreWriteBase): else: return [] - def has_path_to_root(self, block_key, course): + def build_block_key_to_parents_mapping(self, structure): + """ + Given a structure, builds block_key to parents mapping for all block keys in structure + and returns it + + :param structure: db json of course structure + + :return dict: a dictionary containing mapping of block_keys against their parents. + """ + children_to_parents = defaultdict(list) + for parent_key, value in structure['blocks'].iteritems(): + for child_key in value.fields.get('children', []): + children_to_parents[child_key].append(parent_key) + + return children_to_parents + + def has_path_to_root(self, block_key, course, path_cache=None, parents_cache=None): """ Check recursively if an xblock has a path to the course root :param block_key: BlockKey of the component whose path is to be checked :param course: actual db json of course from structures + :param path_cache: a dictionary that records which modules have a path to the root so that we don't have to + double count modules if we're computing this for a list of modules in a course. + :param parents_cache: a dictionary containing mapping of block_key to list of its parents. Optionally, this + should be built for course structure to make this method faster. :return Bool: whether or not component has path to the root """ - xblock_parents = self._get_parents_from_structure(block_key, course.structure) + if path_cache and block_key in path_cache: + return path_cache[block_key] + + if parents_cache is None: + xblock_parents = self._get_parents_from_structure(block_key, course.structure) + else: + xblock_parents = parents_cache[block_key] + if len(xblock_parents) == 0 and block_key.type in ["course", "library"]: # Found, xblock has the path to the root + if path_cache is not None: + path_cache[block_key] = True + return True - return any(self.has_path_to_root(xblock_parent, course) for xblock_parent in xblock_parents) + has_path = any( + self.has_path_to_root(xblock_parent, course, path_cache, parents_cache) + for xblock_parent in xblock_parents + ) + + if path_cache is not None: + path_cache[block_key] = has_path + + return has_path def get_parent_location(self, locator, **kwargs): """ diff --git a/common/lib/xmodule/xmodule/modulestore/store_utilities.py b/common/lib/xmodule/xmodule/modulestore/store_utilities.py index 1dbf3693f4..691ce7941d 100644 --- a/common/lib/xmodule/xmodule/modulestore/store_utilities.py +++ b/common/lib/xmodule/xmodule/modulestore/store_utilities.py @@ -3,6 +3,9 @@ import logging from collections import namedtuple import uuid +from xblock.core import XBlock + +DETACHED_XBLOCK_TYPES = set(name for name, __ in XBlock.load_tagged_classes("detached")) def _prefix_only_url_replace_regex(pattern): 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 84387c3998..95c30d92de 100644 --- a/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py +++ b/common/lib/xmodule/xmodule/modulestore/tests/test_mixed_modulestore.py @@ -22,8 +22,6 @@ from pytz import UTC from shutil import rmtree from tempfile import mkdtemp -from xblock.core import XBlock - from xmodule.x_module import XModuleMixin from xmodule.modulestore.edit_info import EditInfoMixin from xmodule.modulestore.inheritance import InheritanceMixin @@ -44,6 +42,7 @@ from xmodule.modulestore.draft_and_published import UnsupportedRevisionError, DI from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseError, ReferentialIntegrityError, NoPathToItem from xmodule.modulestore.mixed import MixedModuleStore from xmodule.modulestore.search import path_to_location, navigation_index +from xmodule.modulestore.store_utilities import DETACHED_XBLOCK_TYPES from xmodule.modulestore.tests.factories import check_mongo_calls, check_exact_number_of_calls, \ mongo_uses_error_check from xmodule.modulestore.tests.utils import create_modulestore_instance, LocationMixin, mock_tab_from_json @@ -463,16 +462,13 @@ class TestMixedModuleStore(CommonMixedModuleStoreSetup): test_course = self.store.create_course('testx', 'GreekHero', 'test_run', self.user_id) course_key = test_course.id - # get detached category list - detached_categories = [name for name, __ in XBlock.load_tagged_classes("detached")] - items = self.store.get_items(course_key) # Check items found are either course or about type self.assertTrue(set(['course', 'about']).issubset(set([item.location.block_type for item in items]))) # Assert that about is a detached category found in get_items self.assertIn( [item.location.block_type for item in items if item.location.block_type == 'about'][0], - detached_categories + DETACHED_XBLOCK_TYPES ) self.assertEqual(len(items), 2)