From 7dcb1bf7c64f7c9ea2de847c09be3c63e3a6da12 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 25 Mar 2013 16:09:13 -0400 Subject: [PATCH 1/3] it appears we are taking one too many round trips to do when pre-fetching children. This can be very expensive as the tree gets wider the deeper we go. For example, in courseware we want depth=2 (course, chapter, sequential). But looking at log output we were also getting verticals, which there can be a lot of. This should cut down on the total data we are grabbing from the DB. --- common/lib/xmodule/xmodule/modulestore/mongo.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/common/lib/xmodule/xmodule/modulestore/mongo.py b/common/lib/xmodule/xmodule/modulestore/mongo.py index f6fa98fc28..b76251bb99 100644 --- a/common/lib/xmodule/xmodule/modulestore/mongo.py +++ b/common/lib/xmodule/xmodule/modulestore/mongo.py @@ -366,6 +366,9 @@ class MongoModuleStore(ModuleStoreBase): children.extend(item.get('definition', {}).get('children', [])) data[Location(item['location'])] = item + if depth == 0: + break; + # Load all children by id. See # http://www.mongodb.org/display/DOCS/Advanced+Queries#AdvancedQueries-%24or # for or-query syntax From d8f1c2b41a7b1f0af023f8dd75048f31d3df8569 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 25 Mar 2013 22:49:39 -0400 Subject: [PATCH 2/3] add unit test for proper depth build out --- .../contentstore/tests/test_contentstore.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 615ffb6ed0..5b080c7a83 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -205,7 +205,7 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): new_loc = descriptor.location._replace(org='MITx', course='999') print "Checking {0} should now also be at {1}".format(descriptor.location.url(), new_loc.url()) resp = self.client.get(reverse('edit_unit', kwargs={'location': new_loc.url()})) - self.assertEqual(resp.status_code, 200) + self.assertEqual(resp.status_code, 200) def test_delete_course(self): import_from_xml(modulestore(), 'common/test/data/', ['full']) @@ -307,6 +307,21 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # note, we know the link it should be because that's what in the 'full' course in the test data self.assertContains(resp, '/c4x/edX/full/asset/handouts_schematic_tutorial.pdf') + def test_prefetch_children(self): + import_from_xml(modulestore(), 'common/test/data/', ['full']) + module_store = modulestore('direct') + location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') + + course = module_store.get_item(location, depth=2) + + # make sure we pre-fetched a known sequential which should be at depth=2 + self.assertTrue(Location(['i4x', 'edX', 'full', 'sequential', + 'Administrivia_and_Circuit_Elements', None]) in course.system.module_data) + + # make sure we don't have a specific vertical which should be at depth=3 + self.assertFalse(Location(['i4x', 'edX', 'full', 'vertical', 'vertical_58', + None]) in course.system.module_data) + def test_export_course_with_unknown_metadata(self): module_store = modulestore('direct') content_store = contentstore() From 269152c4f2072decad55615c3fc7f612cce28075 Mon Sep 17 00:00:00 2001 From: Chris Dodge Date: Mon, 25 Mar 2013 23:15:35 -0400 Subject: [PATCH 3/3] add a test scenario to count RT to database when prefetching children. This uses a shim function on pymongo's collection.find to do the counting --- .../contentstore/tests/test_contentstore.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/tests/test_contentstore.py b/cms/djangoapps/contentstore/tests/test_contentstore.py index 5b080c7a83..edb20561bc 100644 --- a/cms/djangoapps/contentstore/tests/test_contentstore.py +++ b/cms/djangoapps/contentstore/tests/test_contentstore.py @@ -37,6 +37,14 @@ TEST_DATA_MODULESTORE = copy.deepcopy(settings.MODULESTORE) TEST_DATA_MODULESTORE['default']['OPTIONS']['fs_root'] = path('common/test/data') TEST_DATA_MODULESTORE['direct']['OPTIONS']['fs_root'] = path('common/test/data') +class MongoCollectionFindWrapper(object): + def __init__(self, original): + self.original = original + self.counter = 0 + + def find(self, query, *args, **kwargs): + self.counter = self.counter+1 + return self.original(query, *args, **kwargs) @override_settings(MODULESTORE=TEST_DATA_MODULESTORE) class ContentStoreToyCourseTest(ModuleStoreTestCase): @@ -145,8 +153,6 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): # make sure the parent no longer points to the child object which was deleted self.assertFalse(sequential.location.url() in chapter.children) - - def test_about_overrides(self): ''' This test case verifies that a course can use specialized override for about data, e.g. /about/Fall_2012/effort.html @@ -312,8 +318,15 @@ class ContentStoreToyCourseTest(ModuleStoreTestCase): module_store = modulestore('direct') location = CourseDescriptor.id_to_location('edX/full/6.002_Spring_2012') + wrapper = MongoCollectionFindWrapper(module_store.collection.find) + module_store.collection.find = wrapper.find course = module_store.get_item(location, depth=2) + # make sure we haven't done too many round trips to DB + # note we say 4 round trips here for 1) the course, 2 & 3) for the chapters and sequentials, and + # 4) because of the RT due to calculating the inherited metadata + self.assertEqual(wrapper.counter, 4) + # make sure we pre-fetched a known sequential which should be at depth=2 self.assertTrue(Location(['i4x', 'edX', 'full', 'sequential', 'Administrivia_and_Circuit_Elements', None]) in course.system.module_data)