[GCU] Loading yang-models only once (#1981)

#### What I did Loading sonic-yang models only once, and re-using them. This makes the sorting a lot faster. How to verify `loadYangModel` took a lot of time? Add the following snippet to `TestPatchSorter` ```python from pstats import Stats import cProfile class TestPatchSorter(...): def setUp(self): """init each test""" self.pr = cProfile.Profile() self.pr.enable() print("\n<<<---") def tearDown(self): """finish any test""" p = Stats (self.pr) p.strip_dirs() p.sort_stats ('cumtime') p.print_stats () print("\n--->>>") . . . # Also update verify(self, cc_ops=[], tc_ops=[]) by commenting out changes validation to avoid extra calls to loadYangModels def verify(self, cc_ops=[], tc_ops=[]): # Arrange config_wrapper=ConfigWrapper() target_config=jsonpatch.JsonPatch(tc_ops).apply(Files.CROPPED_CONFIG_DB_AS_JSON) current_config=jsonpatch.JsonPatch(cc_ops).apply(Files.CROPPED_CONFIG_DB_AS_JSON) patch=jsonpatch.make_patch(current_config, target_config) # Act actual = self.create_patch_sorter(current_config).sort(patch) # Assert # simulated_config = current_config # for move in actual: # simulated_config = move.apply(simulated_config) # self.assertTrue(config_wrapper.validate_config_db_config(simulated_config)) # self.assertEqual(target_config, simulated_config) ``` Run ``` > python3 -m unittest patch_sorter_test.TestPatchSorter.test_sort__dpb_1_to_4__success . . . 48986582 function calls (48933431 primitive calls) in 104.530 seconds Ordered by: cumulative time ncalls tottime percall cumtime percall filename:lineno(function) 1 0.000 0.000 104.530 104.530 case.py:549(_callTestMethod) 1 0.000 0.000 104.530 104.530 patch_sorter_test.py:1889(test_sort__dpb_1_to_4__success) 1 0.000 0.000 104.529 104.529 patch_sorter_test.py:1933(verify) 1 0.005 0.005 104.527 104.527 patch_sorter.py:1332(sort) 32/1 0.006 0.000 104.077 104.077 patch_sorter.py:955(sort) 334 0.012 0.000 99.498 0.298 patch_sorter.py:310(validate) 492 2.140 0.004 95.810 0.195 sonic_yang_ext.py:30(loadYangModel) <========= ``` From the above we can see profiling data about test_sort__dpb_1_to_4__success: - Took 104.53s to complete - loadYangModel was called 492 times - loadYangModel took 95.810s. loadYangModel is the method that loads the yang models from memory into SonicYang object. The loading of the YANG models should only happen once. #### How I did it Moved all calls to create sonic_yang object to ConfigWrapper, and each call to create a new instance just fills in the data for the yang models. #### How to verify it unit-test Running profiling after the update: ``` > python3 -m unittest patch_sorter_test.TestPatchSorter.test_sort__dpb_1_to_4__success . . . 702096 function calls (648951 primitive calls) in 2.882 seconds Ordered by: cumulative time ncalls tottime percall cumtime percall filename:lineno(function) 1 0.000 0.000 2.882 2.882 case.py:549(_callTestMethod) 1 0.000 0.000 2.882 2.882 patch_sorter_test.py:1890(test_sort__dpb_1_to_4__success) 1 0.002 0.002 2.881 2.881 patch_sorter_test.py:1934(verify) 1 0.000 0.000 2.874 2.874 patch_sorter.py:1332(sort) 32/1 0.004 0.000 2.705 2.705 patch_sorter.py:955(sort) 334 0.008 0.000 2.242 0.007 patch_sorter.py:310(validate) 490 0.080 0.000 1.791 0.004 sonic_yang_ext.py:1043(loadData) 332 0.043 0.000 1.655 0.005 patch_sorter.py:345(validate) 332 0.018 0.000 1.509 0.005 gu_common.py:112(validate_config_db_config) . . . 1 0.002 0.002 0.164 0.164 sonic_yang_ext.py:30(loadYangModel) ``` From the above we can see profiling data about test_sort__dpb_1_to_4__success: - Took 2.882s to complete - loadYangModel was called 1time - loadYangModel took 0.164s. [profiling-after.txt](https://github.com/Azure/sonic-utilities/files/7757252/profiling-after.txt) #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed)
sonic-net · Dec 22, 2021 · 3b642c9 · 3b642c9
1 parent bb56fc2
commit 3b642c9
Show file tree

Hide file tree

Showing 4 changed files with 75 additions and 22 deletions.
diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py
@@ -43,6 +43,7 @@ def __eq__(self, other):
 class ConfigWrapper:
     def __init__(self, yang_dir = YANG_DIR):
         self.yang_dir = YANG_DIR
+        self.sonic_yang_with_loaded_models = None
 
     def get_config_db_as_json(self):
         text = self._get_config_db_as_text()
@@ -63,8 +64,7 @@ def get_sonic_yang_as_json(self):
         return self.convert_config_db_to_sonic_yang(config_db_json)
 
     def convert_config_db_to_sonic_yang(self, config_db_as_json):
-        sy = sonic_yang.SonicYang(self.yang_dir)
-        sy.loadYangModel()
+        sy = self.create_sonic_yang_with_loaded_models()
 
         # Crop config_db tables that do not have sonic yang models
         cropped_config_db_as_json = self.crop_tables_without_yang(config_db_as_json)
@@ -76,8 +76,7 @@ def convert_config_db_to_sonic_yang(self, config_db_as_json):
         return sonic_yang_as_json
 
     def convert_sonic_yang_to_config_db(self, sonic_yang_as_json):
-        sy = sonic_yang.SonicYang(self.yang_dir)
-        sy.loadYangModel()
+        sy = self.create_sonic_yang_with_loaded_models()
 
         # replace container of the format 'module:table' with just 'table'
         new_sonic_yang_json = {}
@@ -100,8 +99,7 @@ def convert_sonic_yang_to_config_db(self, sonic_yang_as_json):
     def validate_sonic_yang_config(self, sonic_yang_as_json):
         config_db_as_json = self.convert_sonic_yang_to_config_db(sonic_yang_as_json)
 
-        sy = sonic_yang.SonicYang(self.yang_dir)
-        sy.loadYangModel()
+        sy = self.create_sonic_yang_with_loaded_models()
 
         try:
             sy.loadData(config_db_as_json)
@@ -112,8 +110,7 @@ def validate_sonic_yang_config(self, sonic_yang_as_json):
             return False
 
     def validate_config_db_config(self, config_db_as_json):
-        sy = sonic_yang.SonicYang(self.yang_dir)
-        sy.loadYangModel()
+        sy = self.create_sonic_yang_with_loaded_models()
 
         try:
             tmp_config_db_as_json = copy.deepcopy(config_db_as_json)
@@ -126,8 +123,7 @@ def validate_config_db_config(self, config_db_as_json):
             return False
 
     def crop_tables_without_yang(self, config_db_as_json):
-        sy = sonic_yang.SonicYang(self.yang_dir)
-        sy.loadYangModel()
+        sy = self.create_sonic_yang_with_loaded_models()
 
         sy.jIn = copy.deepcopy(config_db_as_json)
 
@@ -151,6 +147,16 @@ def remove_empty_tables(self, config):
                 config_with_non_empty_tables[table] = copy.deepcopy(config[table])
         return config_with_non_empty_tables
 
+    # TODO: move creating copies of sonic_yang with loaded models to sonic-yang-mgmt directly
+    def create_sonic_yang_with_loaded_models(self):
+        # sonic_yang_with_loaded_models will only be initialized once the first time this method is called
+        if self.sonic_yang_with_loaded_models is None:
+            loaded_models_sy = sonic_yang.SonicYang(self.yang_dir)
+            loaded_models_sy.loadYangModel() # This call takes a long time (100s of ms) because it reads files from disk
+            self.sonic_yang_with_loaded_models = loaded_models_sy
+
+        return copy.copy(self.sonic_yang_with_loaded_models)
+
 class DryRunConfigWrapper(ConfigWrapper):
     # This class will simulate all read/write operations to ConfigDB on a virtual storage unit.
     def __init__(self, initial_imitated_config_db = None):
@@ -176,7 +182,7 @@ def _init_imitated_config_db_if_none(self):
 class PatchWrapper:
     def __init__(self, config_wrapper=None):
         self.config_wrapper = config_wrapper if config_wrapper is not None else ConfigWrapper()
-        self.path_addressing = PathAddressing()
+        self.path_addressing = PathAddressing(self.config_wrapper)
 
     def validate_config_db_patch_has_yang_models(self, patch):
         config_db = {}
@@ -256,6 +262,10 @@ class PathAddressing:
     """
     PATH_SEPARATOR = "/"
     XPATH_SEPARATOR = "/"
+
+    def __init__(self, config_wrapper=None):
+        self.config_wrapper = config_wrapper
+
     def get_path_tokens(self, path):
         return JsonPointer(path).parts
 
@@ -391,8 +401,7 @@ def find_ref_paths(self, path, config):
         return self._find_leafref_paths(path, config)
 
     def _find_leafref_paths(self, path, config):
-        sy = sonic_yang.SonicYang(YANG_DIR)
-        sy.loadYangModel()
+        sy = self.config_wrapper.create_sonic_yang_with_loaded_models()
 
         sy.loadData(config)
 

diff --git a/generic_config_updater/patch_sorter.py b/generic_config_updater/patch_sorter.py
@@ -1119,21 +1119,21 @@ class IgnorePathsFromYangConfigSplitter:
     def __init__(self, ignore_paths_from_yang_list, config_wrapper):
         self.ignore_paths_from_yang_list = ignore_paths_from_yang_list
         self.config_wrapper = config_wrapper
+        self.path_addressing = PathAddressing(config_wrapper)
 
     def split_yang_non_yang_distinct_field_path(self, config):
         config_with_yang = copy.deepcopy(config)
         config_without_yang = {}
 
-        path_addressing = PathAddressing()
         # ignore more config from config_with_yang
         for path in self.ignore_paths_from_yang_list:
-            if not path_addressing.has_path(config_with_yang, path):
+            if not self.path_addressing.has_path(config_with_yang, path):
                 continue
             if path == '': # whole config to be ignored
                 return {}, copy.deepcopy(config)
 
             # Add to config_without_yang from config_with_yang
-            tokens = path_addressing.get_path_tokens(path)
+            tokens = self.path_addressing.get_path_tokens(path)
             add_move = JsonMove(Diff(config_without_yang, config_with_yang), OperationType.ADD, tokens, tokens)
             config_without_yang = add_move.apply(config_without_yang)
 
@@ -1325,7 +1325,7 @@ def __init__(self, config_wrapper, patch_wrapper, sort_algorithm_factory=None):
         self.config_wrapper = config_wrapper
         self.patch_wrapper = patch_wrapper
         self.operation_wrapper = OperationWrapper()
-        self.path_addressing = PathAddressing()
+        self.path_addressing = PathAddressing(self.config_wrapper)
         self.sort_algorithm_factory = sort_algorithm_factory if sort_algorithm_factory else \
             SortAlgorithmFactory(self.operation_wrapper, config_wrapper, self.path_addressing)
 

diff --git a/tests/generic_config_updater/gu_common_test.py b/tests/generic_config_updater/gu_common_test.py
@@ -258,6 +258,49 @@ def test_remove_empty_tables__multiple_empty_tables__returns_config_without_empt
         # Assert
         self.assertDictEqual({"any_table": {"key": "value"}}, actual)
 
+    def test_create_sonic_yang_with_loaded_models__creates_new_sonic_yang_every_call(self):
+        # check yang models fields are the same or None, non-yang model fields are different
+        def check(sy1, sy2):
+            # instances are different
+            self.assertNotEqual(sy1, sy2)
+
+            # yang models fields are same or None
+            self.assertTrue(sy1.confDbYangMap is sy2.confDbYangMap)
+            self.assertTrue(sy1.ctx is sy2.ctx)
+            self.assertTrue(sy1.DEBUG is sy2.DEBUG)
+            self.assertTrue(sy1.preProcessedYang is sy2.preProcessedYang)
+            self.assertTrue(sy1.SYSLOG_IDENTIFIER is sy2.SYSLOG_IDENTIFIER)
+            self.assertTrue(sy1.yang_dir is sy2.yang_dir)
+            self.assertTrue(sy1.yangFiles is sy2.yangFiles)
+            self.assertTrue(sy1.yJson is sy2.yJson)
+            self.assertTrue(not(hasattr(sy1, 'module')) or sy1.module is None) # module is unused, might get deleted
+            self.assertTrue(not(hasattr(sy2, 'module')) or sy2.module is None)
+
+            # non yang models fields are different
+            self.assertFalse(sy1.root is sy2.root)
+            self.assertFalse(sy1.jIn is sy2.jIn)
+            self.assertFalse(sy1.tablesWithOutYang is sy2.tablesWithOutYang)
+            self.assertFalse(sy1.xlateJson is sy2.xlateJson)
+            self.assertFalse(sy1.revXlateJson is sy2.revXlateJson)
+
+        config_wrapper = gu_common.ConfigWrapper()
+        self.assertTrue(config_wrapper.sonic_yang_with_loaded_models is None)
+
+        sy1 = config_wrapper.create_sonic_yang_with_loaded_models()
+        sy2 = config_wrapper.create_sonic_yang_with_loaded_models()
+
+        # Simulating loading non-yang model fields
+        sy1.loadData(Files.ANY_CONFIG_DB)
+        sy1.getData()
+
+        # Simulating loading non-yang model fields
+        sy2.loadData(Files.ANY_CONFIG_DB)
+        sy2.getData()
+
+        check(sy1, sy2)
+        check(sy1, config_wrapper.sonic_yang_with_loaded_models)
+        check(sy2, config_wrapper.sonic_yang_with_loaded_models)
+
 class TestPatchWrapper(unittest.TestCase):
     def setUp(self):
         self.config_wrapper_mock = gu_common.ConfigWrapper()
@@ -443,7 +486,7 @@ def __assert_same_patch(self, config_db_patch, sonic_yang_patch, config_wrapper,
 
 class TestPathAddressing(unittest.TestCase):
     def setUp(self):
-        self.path_addressing = gu_common.PathAddressing()
+        self.path_addressing = gu_common.PathAddressing(gu_common.ConfigWrapper())
         self.sy_only_models = sonic_yang.SonicYang(gu_common.YANG_DIR)
         self.sy_only_models.loadYangModel()
 

diff --git a/tests/generic_config_updater/patch_sorter_test.py b/tests/generic_config_updater/patch_sorter_test.py
@@ -921,8 +921,8 @@ def verify_diff(self, current_config, target_config, current_config_tokens=None,
 
 class TestNoDependencyMoveValidator(unittest.TestCase):
     def setUp(self):
-        path_addressing = ps.PathAddressing()
         config_wrapper = ConfigWrapper()
+        path_addressing = ps.PathAddressing(config_wrapper)
         self.validator = ps.NoDependencyMoveValidator(path_addressing, config_wrapper)
 
     def test_validate__add_full_config_has_dependencies__failure(self):
@@ -1649,7 +1649,7 @@ def verify_moves(self, ex_ops, moves):
 
 class DeleteRefsMoveExtender(unittest.TestCase):
     def setUp(self):
-        self.extender = ps.DeleteRefsMoveExtender(PathAddressing())
+        self.extender = ps.DeleteRefsMoveExtender(PathAddressing(ConfigWrapper()))
 
     def test_extend__non_delete_ops__no_extended_moves(self):
         self.verify(OperationType.ADD,
@@ -1723,7 +1723,8 @@ def test_memoization_sorter(self):
 
     def verify(self, algo, algo_class):
         # Arrange
-        factory = ps.SortAlgorithmFactory(OperationWrapper(), ConfigWrapper(), PathAddressing())
+        config_wrapper = ConfigWrapper()
+        factory = ps.SortAlgorithmFactory(OperationWrapper(), config_wrapper, PathAddressing(config_wrapper))
         expected_generators = [ps.LowLevelMoveGenerator]
         expected_extenders = [ps.UpperLevelMoveExtender, ps.DeleteInsteadOfReplaceMoveExtender, ps.DeleteRefsMoveExtender]
         expected_validator = [ps.DeleteWholeConfigMoveValidator,
@@ -1834,7 +1835,7 @@ def create_patch_sorter(self, config=None):
         config_wrapper.get_config_db_as_json = MagicMock(return_value=config)
         patch_wrapper = PatchWrapper(config_wrapper)
         operation_wrapper = OperationWrapper()
-        path_addressing= ps.PathAddressing()
+        path_addressing= ps.PathAddressing(config_wrapper)
         sort_algorithm_factory = ps.SortAlgorithmFactory(operation_wrapper, config_wrapper, path_addressing)
 
         return ps.PatchSorter(config_wrapper, patch_wrapper, sort_algorithm_factory)