From 3b642c98fb1deeb16a62ff288dfc5e9a91c9676b Mon Sep 17 00:00:00 2001 From: Mohamed Ghoneim Date: Wed, 22 Dec 2021 07:14:30 -0800 Subject: [PATCH] [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) --- generic_config_updater/gu_common.py | 35 +++++++++------ generic_config_updater/patch_sorter.py | 8 ++-- .../generic_config_updater/gu_common_test.py | 45 ++++++++++++++++++- .../patch_sorter_test.py | 9 ++-- 4 files changed, 75 insertions(+), 22 deletions(-) diff --git a/generic_config_updater/gu_common.py b/generic_config_updater/gu_common.py index 8f8f509e177f..d7eef0f1b141 100644 --- 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 index b795421064f1..3cd9e757fa45 100644 --- 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 index 7d0c9124bce9..52cf9a818de1 100644 --- 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 index 4ac0c574a4a1..710de1860d3e 100644 --- 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)