Reduce number of transformer traversals (#12881)

This change improves grading performance by combining what had previously been
multiple course tree traversals into a single traversal with combined
filtering logic.

lms/djangoapps/course_api/blocks/transformers/block_depth.py

@@ -58,6 +58,6 @@ class BlockDepthTransformer(BlockStructureTransformer):
             )
 
         if self.requested_depth is not None:
-            block_structure.remove_block_if(
+            block_structure.remove_block_traversal(
                 lambda block_key: self.get_block_depth(block_structure, block_key) > self.requested_depth
             )

lms/djangoapps/course_api/blocks/transformers/proctored_exam.py

@@ -6,10 +6,10 @@ from django.conf import settings
 
 from edx_proctoring.api import get_attempt_status_summary
 from edx_proctoring.models import ProctoredExamStudentAttemptStatus
-from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
+from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin
 
 
-class ProctoredExamTransformer(BlockStructureTransformer):
+class ProctoredExamTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
     Exclude proctored exams unless the user is not a verified student or has
     declined taking the exam.
@@ -33,12 +33,9 @@ class ProctoredExamTransformer(BlockStructureTransformer):
         block_structure.request_xblock_fields('is_proctored_enabled')
         block_structure.request_xblock_fields('is_practice_exam')
 
-    def transform(self, usage_info, block_structure):
-        """
-        Mutates block_structure based on the given usage_info.
-        """
+    def transform_block_filters(self, usage_info, block_structure):
         if not settings.FEATURES.get('ENABLE_PROCTORED_EXAMS', False):
-            return
+            return [block_structure.create_universal_filter()]
 
         def is_proctored_exam_for_user(block_key):
             """
@@ -60,4 +57,4 @@ class ProctoredExamTransformer(BlockStructureTransformer):
                 )
                 return user_exam_summary and user_exam_summary['status'] != ProctoredExamStudentAttemptStatus.declined
 
-        block_structure.remove_block_if(is_proctored_exam_for_user)
+        return [block_structure.create_removal_filter(is_proctored_exam_for_user)]

lms/djangoapps/course_blocks/transformers/library_content.py

@@ -3,13 +3,13 @@ Content Library Transformer.
 """
 import json
 from courseware.models import StudentModule
-from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
+from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin
 from xmodule.library_content_module import LibraryContentModule
 from xmodule.modulestore.django import modulestore
 from eventtracking import tracker
 
 
-class ContentLibraryTransformer(BlockStructureTransformer):
+class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
     A transformer that manipulates the block structure by removing all
     blocks within a library_content module to which a user should not
@@ -59,17 +59,12 @@ class ContentLibraryTransformer(BlockStructureTransformer):
                 summary = summarize_block(child_key)
                 block_structure.set_transformer_block_field(child_key, cls, 'block_analytics_summary', summary)
 
-    def transform(self, usage_info, block_structure):
-        """
-        Mutates block_structure based on the given usage_info.
-        """
-
+    def transform_block_filters(self, usage_info, block_structure):
         all_library_children = set()
         all_selected_children = set()
-        for block_key in block_structure.topological_traversal(
-                filter_func=lambda block_key: block_key.block_type == 'library_content',
-                yield_descendants_of_unyielded=True,
-        ):
+        for block_key in block_structure:
+            if block_key.block_type != 'library_content':
+                continue
             library_children = block_structure.get_children(block_key)
             if library_children:
                 all_library_children.update(library_children)
@@ -110,11 +105,7 @@ class ContentLibraryTransformer(BlockStructureTransformer):
                 return False
             return True
 
-        # Check and remove all non-selected children from course
-        # structure.
-        block_structure.remove_block_if(
-            check_child_removal
-        )
+        return [block_structure.create_removal_filter(check_child_removal)]
 
     @classmethod
     def _get_student_module(cls, user, course_key, block_key):

lms/djangoapps/course_blocks/transformers/split_test.py

 """
 Split Test Block Transformer
 """
-from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
+from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin
 
 
-class SplitTestTransformer(BlockStructureTransformer):
+class SplitTestTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
     A nested transformer of the UserPartitionTransformer that honors the
     block structure pathways created by split_test modules.
@@ -68,14 +68,16 @@ class SplitTestTransformer(BlockStructureTransformer):
                 group = child_to_group.get(child_location, None)
                 child.group_access[partition_for_this_block.id] = [group] if group is not None else []
 
-    def transform(self, usage_info, block_structure):
+    def transform_block_filters(self, usage_info, block_structure):
         """
         Mutates block_structure based on the given usage_info.
         """
 
         # The UserPartitionTransformer will enforce group access, so
         # go ahead and remove all extraneous split_test modules.
-        block_structure.remove_block_if(
-            lambda block_key: block_key.block_type == 'split_test',
-            keep_descendants=True,
-        )
+        return [
+            block_structure.create_removal_filter(
+                lambda block_key: block_key.block_type == 'split_test',
+                keep_descendants=True,
+            )
+        ]

lms/djangoapps/course_blocks/transformers/start_date.py

 """
 Start Date Transformer implementation.
 """
-from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
+from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin
 from lms.djangoapps.courseware.access_utils import check_start_date
 from xmodule.course_metadata_utils import DEFAULT_START_DATE
 
 from .utils import get_field_on_block
 
 
-class StartDateTransformer(BlockStructureTransformer):
+class StartDateTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
     A transformer that enforces the 'start' and 'days_early_for_beta'
     fields on blocks by removing blocks from the block structure for
@@ -83,19 +83,15 @@ class StartDateTransformer(BlockStructureTransformer):
                 merged_start_value
             )
 
-    def transform(self, usage_info, block_structure):
-        """
-        Mutates block_structure based on the given usage_info.
-        """
+    def transform_block_filters(self, usage_info, block_structure):
         # Users with staff access bypass the Start Date check.
         if usage_info.has_staff_access:
-            return
-
-        block_structure.remove_block_if(
-            lambda block_key: not check_start_date(
-                usage_info.user,
-                block_structure.get_xblock_field(block_key, 'days_early_for_beta'),
-                self.get_merged_start_date(block_structure, block_key),
-                usage_info.course_key,
-            )
+            return [block_structure.create_universal_filter()]
+
+        removal_condition = lambda block_key: not check_start_date(
+            usage_info.user,
+            block_structure.get_xblock_field(block_key, 'days_early_for_beta'),
+            self.get_merged_start_date(block_structure, block_key),
+            usage_info.course_key,
         )
+        return [block_structure.create_removal_filter(removal_condition)]

lms/djangoapps/course_blocks/transformers/user_partitions.py

 """
 User Partitions Transformer
 """
-from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
+from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin
 
 from .split_test import SplitTestTransformer
 from .utils import get_field_on_block
 
 
-class UserPartitionTransformer(BlockStructureTransformer):
+class UserPartitionTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
     A transformer that enforces the group access rules on course blocks,
     by honoring their user_partitions and group_access fields, and
@@ -64,26 +64,25 @@ class UserPartitionTransformer(BlockStructureTransformer):
             merged_group_access = _MergedGroupAccess(user_partitions, xblock, merged_parent_access_list)
             block_structure.set_transformer_block_field(block_key, cls, 'merged_group_access', merged_group_access)
 
-    def transform(self, usage_info, block_structure):
-        """
-        Mutates block_structure based on the given usage_info.
-        """
-        SplitTestTransformer().transform(usage_info, block_structure)
+    def transform_block_filters(self, usage_info, block_structure):
+        result_list = SplitTestTransformer().transform_block_filters(usage_info, block_structure)
 
         user_partitions = block_structure.get_transformer_data(self, 'user_partitions')
-
         if not user_partitions:
-            return
+            return [block_structure.create_universal_filter()]
 
         user_groups = _get_user_partition_groups(
             usage_info.course_key, user_partitions, usage_info.user
         )
-        block_structure.remove_block_if(
+        group_access_filter = block_structure.create_removal_filter(
             lambda block_key: not block_structure.get_transformer_block_field(
                 block_key, self, 'merged_group_access'
             ).check_group_access(user_groups)
         )
 
+        result_list.append(group_access_filter)
+        return result_list
+
 
 class _MergedGroupAccess(object):
     """

lms/djangoapps/course_blocks/transformers/visibility.py

 """
 Visibility Transformer implementation.
 """
-from openedx.core.lib.block_structure.transformer import BlockStructureTransformer
+from openedx.core.lib.block_structure.transformer import BlockStructureTransformer, FilteringTransformerMixin
 
 
-class VisibilityTransformer(BlockStructureTransformer):
+class VisibilityTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
     A transformer that enforces the visible_to_staff_only field on
     blocks by removing blocks from the block structure for which the
@@ -67,14 +67,13 @@ class VisibilityTransformer(BlockStructureTransformer):
                 )
             )
 
-    def transform(self, usage_info, block_structure):
-        """
-        Mutates block_structure based on the given usage_info.
-        """
+    def transform_block_filters(self, usage_info, block_structure):
         # Users with staff access bypass the Visibility check.
         if usage_info.has_staff_access:
-            return
+            return [block_structure.create_universal_filter()]
 
-        block_structure.remove_block_if(
-            lambda block_key: self.get_visible_to_staff_only(block_structure, block_key)
-        )
+        return [
+            block_structure.create_removal_filter(
+                lambda block_key: self.get_visible_to_staff_only(block_structure, block_key),
+            )
+        ]

openedx/core/lib/block_structure/block_structure.py

@@ -8,6 +8,7 @@ The following internal data structures are implemented:
     _BlockRelations - Data structure for a single block's relations.
     _BlockData - Data structure for a single block's data.
 """
+from functools import partial
 from logging import getLogger
 
 from openedx.core.lib.graph_traversals import traverse_topologically, traverse_post_order
@@ -608,11 +609,57 @@ class BlockStructureBlockData(BlockStructure):
                 for parent in parents:
                     self._add_relation(parent, child)
 
-    def remove_block_if(self, removal_condition, keep_descendants=False, **kwargs):
+    def create_universal_filter(self):
+        """
+        Returns a filter function that always returns True for all blocks.
+        """
+        return lambda block_key: True
+
+    def create_removal_filter(self, removal_condition, keep_descendants=False):
+        """
+        Returns a filter function that automatically removes blocks that satisfy
+        the removal_condition.
+
+        Arguments:
+            removal_condition ((usage_key)->bool) - A function that
+                takes a block's usage key as input and returns whether
+                or not to remove that block from the block structure.
+
+            keep_descendants (bool) - See the description in
+                remove_block.
+        """
+        return partial(
+            self.retain_or_remove,
+            removal_condition=removal_condition,
+            keep_descendants=keep_descendants,
+        )
+
+    def retain_or_remove(self, block_key, removal_condition, keep_descendants=False):
+        """
+        Removes the given block if it satisfies the removal_condition.
+        Returns True if the block was retained, and False if the block
+        was removed.
+
+        Arguments:
+            block_key (usage_key) - Usage key of the block.
+
+            removal_condition ((usage_key)->bool) - A function that
+                takes a block's usage key as input and returns whether
+                or not to remove that block from the block structure.
+
+            keep_descendants (bool) - See the description in
+                remove_block.
+        """
+        if removal_condition(block_key):
+            self.remove_block(block_key, keep_descendants)
+            return False
+        return True
+
+    def remove_block_traversal(self, removal_condition, keep_descendants=False):
         """
         A higher-order function that traverses the block structure
-        using topological sort and removes any blocks encountered that
-        satisfy the removal_condition.
+        using topological sort and removes all blocks satisfying the given
+        removal_condition.
 
         Arguments:
             removal_condition ((usage_key)->bool) - A function that
@@ -621,19 +668,26 @@ class BlockStructureBlockData(BlockStructure):
 
             keep_descendants (bool) - See the description in
                 remove_block.
+        """
+        self.filter_topological_traversal(
+            filter_func=self.create_removal_filter(
+                removal_condition, keep_descendants
+            )
+        )
+
+    def filter_topological_traversal(self, filter_func, **kwargs):
+        """
+        A higher-order function that traverses the block structure
+        using topological sort and applies the given filter.
+
+        Arguments:
+            filter_func ((usage_key)->bool) - Function that returns
+                whether or not to yield the given block key.
+                If None, the True function is assumed.
 
             kwargs (dict) - Optional keyword arguments to be forwarded
                 to topological_traversal.
         """
-        def filter_func(block_key):
-            """
-            Filter function for removing blocks that satisfy the
-            removal_condition.
-            """
-            if removal_condition(block_key):
-                self.remove_block(block_key, keep_descendants)
-                return False
-            return True
 
         # Note: For optimization, we remove blocks using the filter
         # function, since the graph traversal method can skip over

openedx/core/lib/block_structure/tests/helpers.py

@@ -6,7 +6,7 @@ from mock import patch
 from xmodule.modulestore.exceptions import ItemNotFoundError
 
 from ..block_structure import BlockStructureBlockData
-from ..transformer import BlockStructureTransformer
+from ..transformer import BlockStructureTransformer, FilteringTransformerMixin
 
 
 class MockXBlock(object):
@@ -147,6 +147,21 @@ class MockTransformer(BlockStructureTransformer):
         pass
 
 
+class MockFilteringTransformer(FilteringTransformerMixin, BlockStructureTransformer):
+    """
+    A mock FilteringTransformerMixin class.
+    """
+    VERSION = 1
+
+    @classmethod
+    def name(cls):
+        # Use the class' name for Mock transformers.
+        return cls.__name__
+
+    def transform_block_filters(self, usage_info, block_structure):
+        return [block_structure.create_universal_filter()]
+
+
 @contextmanager
 def mock_registered_transformers(transformers):
     """

openedx/core/lib/block_structure/tests/test_block_structure.py

@@ -217,7 +217,7 @@ class TestBlockStructureData(TestCase, ChildrenMapTestMixin):
 
         self.assert_block_structure(block_structure, pruned_children_map, missing_blocks)
 
-    def test_remove_block_if(self):
+    def test_remove_block_traversal(self):
         block_structure = self.create_block_structure(ChildrenMapTestMixin.LINEAR_CHILDREN_MAP)
-        block_structure.remove_block_if(lambda block: block == 2)
+        block_structure.remove_block_traversal(lambda block: block == 2)
         self.assert_block_structure(block_structure, [[1], [], [], []], missing_blocks=[2])

openedx/core/lib/block_structure/tests/test_transformers.py

@@ -9,7 +9,7 @@ from ..block_structure import BlockStructureModulestoreData
 from ..exceptions import TransformerException
 from ..transformers import BlockStructureTransformers
 from .helpers import (
-    ChildrenMapTestMixin, MockTransformer, mock_registered_transformers
+    ChildrenMapTestMixin, MockTransformer, MockFilteringTransformer, mock_registered_transformers
 )
 
 
@@ -27,7 +27,7 @@ class TestBlockStructureTransformers(ChildrenMapTestMixin, TestCase):
     def setUp(self):
         super(TestBlockStructureTransformers, self).setUp()
         self.transformers = BlockStructureTransformers(usage_info=MagicMock())
-        self.registered_transformers = [MockTransformer]
+        self.registered_transformers = [MockTransformer(), MockFilteringTransformer()]
 
     def add_mock_transformer(self):
         """
@@ -38,13 +38,21 @@ class TestBlockStructureTransformers(ChildrenMapTestMixin, TestCase):
 
     def test_add_registered(self):
         self.add_mock_transformer()
-        self.assertIn(MockTransformer, self.transformers._transformers)  # pylint: disable=protected-access
+        self.assertIn(
+            self.registered_transformers[0],
+            self.transformers._transformers['no_filter']  # pylint: disable=protected-access
+        )
+        self.assertIn(
+            self.registered_transformers[1],
+            self.transformers._transformers['supports_filter']  # pylint: disable=protected-access
+        )
 
     def test_add_unregistered(self):
         with self.assertRaises(TransformerException):
-            self.transformers += [self.UnregisteredTransformer]
+            self.transformers += [self.UnregisteredTransformer()]
 
-        self.assertEquals(self.transformers._transformers, [])  # pylint: disable=protected-access
+        self.assertEquals(self.transformers._transformers['no_filter'], [])  # pylint: disable=protected-access
+        self.assertEquals(self.transformers._transformers['supports_filter'], [])  # pylint: disable=protected-access
 
     def test_collect(self):
         with mock_registered_transformers(self.registered_transformers):

openedx/core/lib/block_structure/transformer.py

@@ -101,15 +101,16 @@ class BlockStructureTransformer(object):
 
         A Transformer may choose to remove entire sub-structures during
         the transform method and may do so using the remove_block and
-        remove_block_if methods.
+        filter_with_removal methods.
 
         Amongst the many methods available for a block_structure, the
         following methods are commonly used during transforms:
             get_xblock_field
             get_transformer_data
             get_transformer_block_field
-            remove_block
-            remove_block_if
+            remove_block_traversal
+            filter_with_removal
+            filter_topological_traversal
             topological_traversal
             post_order_traversal
 
@@ -125,4 +126,57 @@ class BlockStructureTransformer(object):
                 block structure, with already collected data for the
                 transformer, that is to be transformed in place.
         """
-        pass
+        raise NotImplementedError
+
+
+class FilteringTransformerMixin(BlockStructureTransformer):
+    """
+    Transformers may optionally choose to implement this mixin if their
+    transform logic can be broken apart into a lambda for optimization of
+    combined tree traversals.
+
+    For performance reasons, developers should try to implement this mixin
+    whenever possible - with this alternative, traversal of the entire block
+    structure happens only once for all transformers that implement
+    FilteringTransformerMixin.
+    """
+
+    def transform(self, usage_info, block_structure):
+        """
+        By defining this method, FilteringTransformers can be run individually
+        if desired. In normal operations, the filters returned from multiple
+        transform_block_filters calls will be combined and used in a single
+        tree traversal.
+        """
+        block_structure.filter_topological_traversal(self.transform_block_filters(usage_info, block_structure))
+
+    @abstractmethod
+    def transform_block_filters(self, usage_info, block_structure):
+        """
+        This is an alternative to the standard transform method.
+
+        Returns a list of filter functions to be used for filtering out
+        any unwanted blocks in the given block_structure.
+
+        In addition to the commonly used methods listed above, the following
+        methods are commonly used by implementations of transform_block_filters:
+            create_universal_filter
+            create_removal_filter
+
+        Note: Transformers that implement this alternative should be
+        independent of all other registered transformers as they may not
+        be applied in the order in which they were listed in the registry.
+
+        Arguments:
+            usage_info (any negotiated type) - A usage-specific object
+                that is passed to the block_structure and forwarded to all
+                requested Transformers in order to apply a
+                usage-specific transform. For example, an instance of
+                usage_info would contain a user object for which the
+                transform should be applied.
+
+            block_structure (BlockStructureBlockData) - A mutable
+                block structure, with already collected data for the
+                transformer, that is to be transformed in place.
+        """
+        raise NotImplementedError

openedx/core/lib/block_structure/transformers.py

 """
 Module for a collection of BlockStructureTransformers.
 """
+import functools
 from logging import getLogger
 
 from .exceptions import TransformerException
+from .transformer import FilteringTransformerMixin
 from .transformer_registry import TransformerRegistry
 
 
@@ -39,7 +41,7 @@ class BlockStructureTransformers(object):
                 Transformer Registry.
         """
         self.usage_info = usage_info
-        self._transformers = []
+        self._transformers = {'supports_filter': [], 'no_filter': []}
         if transformers:
             self.__iadd__(transformers)
 
@@ -61,7 +63,11 @@ class BlockStructureTransformers(object):
                 "The following requested transformers are not registered: {}".format(unregistered_transformers)
             )
 
-        self._transformers.extend(transformers)
+        for transformer in transformers:
+            if isinstance(transformer, FilteringTransformerMixin):
+                self._transformers['supports_filter'].append(transformer)
+            else:
+                self._transformers['no_filter'].append(transformer)
         return self
 
     @classmethod
@@ -76,17 +82,6 @@ class BlockStructureTransformers(object):
         # Collect all fields that were requested by the transformers.
         block_structure._collect_requested_xblock_fields()  # pylint: disable=protected-access
 
-    def transform(self, block_structure):
-        """
-        The given block structure is transformed by each transformer in the
-        collection, in the order that the transformers were added.
-        """
-        for transformer in self._transformers:
-            transformer.transform(self.usage_info, block_structure)
-
-        # Prune the block structure to remove any unreachable blocks.
-        block_structure._prune_unreachable()  # pylint: disable=protected-access
-
     @classmethod
     def is_collected_outdated(cls, block_structure):
         """
@@ -105,3 +100,50 @@ class BlockStructureTransformers(object):
             )
 
         return bool(outdated_transformers)
+
+    def transform(self, block_structure):
+        """
+        The given block structure is transformed by each transformer in the
+        collection. Tranformers with filters are combined and run first in a
+        single course tree traversal, then remaining transformers are run in
+        the order that they were added.
+        """
+        self._transform_with_filters(block_structure)
+        self._transform_without_filters(block_structure)
+
+        # Prune the block structure to remove any unreachable blocks.
+        block_structure._prune_unreachable()  # pylint: disable=protected-access
+
+    def _transform_with_filters(self, block_structure):
+        """
+        Transforms the given block_structure using the transform_block_filters
+        method from the given transformers.
+        """
+        if not self._transformers['supports_filter']:
+            return
+
+        filters = []
+        for transformer in self._transformers['supports_filter']:
+            filters.extend(transformer.transform_block_filters(self.usage_info, block_structure))
+
+        combined_filters = functools.reduce(
+            self._filter_chain,
+            filters,
+            block_structure.create_universal_filter()
+        )
+        block_structure.filter_topological_traversal(combined_filters)
+
+    def _filter_chain(self, accumulated, additional):
+        """
+        Given two functions that take a block_key and return a boolean, yield
+        a function that takes a block key, and 'ands' the functions together
+        """
+        return lambda block_key: accumulated(block_key) and additional(block_key)
+
+    def _transform_without_filters(self, block_structure):
+        """
+        Transforms the given block_structure using the transform
+        method from the given transformers.
+        """
+        for transformer in self._transformers['no_filter']:
+            transformer.transform(self.usage_info, block_structure)