From f18549f3b5531783c3a2f40fff863fc2dcdb3b77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Trellu?= Date: Mon, 12 Oct 2026 08:38:00 -0400 Subject: [PATCH 1/3] perf: memoize locale directory listings and resource file lookups A skill resolves its resources once per (language, resource type) and once per resource file. Each resolution listed every subdirectory of locale/ and language-matched each one, and every intent, vocab and dialog file was found with its own os.walk of the language directory. On a hub serving 11 languages with skills that ship ~65 locales, that was 26% of the skill loader's wall time (py-spy on a real 18-skill boot; On This Day spent 70% of its load there). Package directories do not change while they are installed, and a reinstall replaces them, so the listings and per-directory file indexes are memoized on (path, inode, mtime), and language matches are lru-cached. The user override directory is still walked on every lookup. When several subdirectories hold the same file name, the index keeps the one the walk reached last, which is what the old loop returned. Co-Authored-By: Claude Opus 5.5 --- ovos_workshop/resource_files.py | 105 +++++++++++++++---- test/unittests/test_resource_lookup_cache.py | 69 ++++++++++++ 2 files changed, 156 insertions(+), 18 deletions(-) create mode 100644 test/unittests/test_resource_lookup_cache.py diff --git a/ovos_workshop/resource_files.py b/ovos_workshop/resource_files.py index 8b3ddabe..9b1b969a 100644 --- a/ovos_workshop/resource_files.py +++ b/ovos_workshop/resource_files.py @@ -17,6 +17,8 @@ import json import re from collections import namedtuple +import os +from functools import lru_cache from os import walk from os.path import dirname from pathlib import Path @@ -93,19 +95,89 @@ def match_lang_directories(lang: str, base_dirs: List[Path]) -> List[Path]: """ candidates = [] for directory in base_dirs: - if directory.exists(): - for folder in directory.iterdir(): - if folder.is_dir(): - # ovos_spec_tools owns the language-distance policy and - # its threshold; a directory whose name is not a usable - # match for `lang` scores beyond it and is skipped. - if lang_matches(lang, folder.name): - candidates.append((folder, lang_distance(lang, folder.name))) + for name in _subdirectory_names(directory): + # ovos_spec_tools owns the language-distance policy and its + # threshold; a directory whose name is not a usable match for + # `lang` scores beyond it and is skipped. + distance = _lang_dir_distance(lang, name) + if distance is not None: + candidates.append((Path(directory, name), distance)) # sort by distance to target lang code candidates = sorted(candidates, key=lambda k: k[1]) return [c[0] for c in candidates] +# A skill resolves its resources once per (language, resource type) and once +# per resource file, and a skill that ships dozens of locales paid for listing +# and language-matching every one of them on each call: on a hub serving 11 +# languages that was ~65 directories x ~26 lookups x 11 languages per skill, +# plus an os.walk per intent/vocab/dialog file. Package directories do not +# change while they are installed, and a reinstall replaces them (new inode, +# new mtime), so both are memoized on (path, inode, mtime). +_DIR_CACHE_MAX = 4096 +_subdir_cache: Dict[str, Tuple[Tuple[int, int], Tuple[str, ...]]] = {} +_file_index_cache: Dict[str, Tuple[Tuple[int, int], Dict[str, Path]]] = {} + + +def _dir_identity(directory) -> Optional[Tuple[int, int]]: + try: + st = os.stat(directory) + except OSError: + return None + return st.st_ino, st.st_mtime_ns + + +def _subdirectory_names(directory) -> Tuple[str, ...]: + """Names of the immediate subdirectories of `directory`, memoized.""" + key = str(directory) + identity = _dir_identity(key) + if identity is None: + return () + cached = _subdir_cache.get(key) + if cached is not None and cached[0] == identity: + return cached[1] + try: + with os.scandir(key) as entries: + names = tuple(sorted(e.name for e in entries if e.is_dir())) + except OSError: + return () + if len(_subdir_cache) >= _DIR_CACHE_MAX: + _subdir_cache.clear() + _subdir_cache[key] = (identity, names) + return names + + +@lru_cache(maxsize=8192) +def _lang_dir_distance(lang: str, name: str) -> Optional[float]: + """lang_distance for a usable language directory name, else None.""" + if not lang_matches(lang, name): + return None + return lang_distance(lang, name) + + +def _walk_file_index(directory) -> Dict[str, Path]: + """file name -> path for every file under `directory`, memoized. + + Reproduces what ResourceFile._locate's walk returned: when several + subdirectories hold the same file name, the one the walk reaches last. + """ + key = str(directory) + identity = _dir_identity(key) + if identity is None: + return {} + cached = _file_index_cache.get(key) + if cached is not None and cached[0] == identity: + return cached[1] + index: Dict[str, Path] = {} + for root, _, file_names in walk(key): + for file_name in file_names: + index[file_name] = Path(root, file_name) + if len(_file_index_cache) >= _DIR_CACHE_MAX: + _file_index_cache.clear() + _file_index_cache[key] = (identity, index) + return index + + def _foreign_lang_dirs(directory: Path, root_dir: str, lang: Optional[str]) -> set: """ @@ -347,19 +419,16 @@ def _locate(self) -> Optional[str]: if file_name in file_names: file_path = Path(directory, file_name) - # check the skill resources - if file_path is None: - walk_directory = str(self.resource_type.base_directory) - for directory, _, file_names in walk(walk_directory): - if file_name in file_names: - file_path = Path(directory, file_name) + # check the skill resources (package data: indexed once per + # directory instead of walked once per resource file) + if file_path is None and self.resource_type.base_directory is not None: + file_path = _walk_file_index( + self.resource_type.base_directory).get(file_name) # check ovos-workshop resources if file_path is None and self.resource_type.workshop_directory is not None: - walk_directory = str(self.resource_type.workshop_directory) - for directory, _, file_names in walk(walk_directory): - if file_name in file_names: - file_path = Path(directory, file_name) + file_path = _walk_file_index( + self.resource_type.workshop_directory).get(file_name) if file_path is None: LOG.debug(f"Could not find resource file {file_name} for lang: {self.resource_type.language}") diff --git a/test/unittests/test_resource_lookup_cache.py b/test/unittests/test_resource_lookup_cache.py new file mode 100644 index 00000000..76ca7699 --- /dev/null +++ b/test/unittests/test_resource_lookup_cache.py @@ -0,0 +1,69 @@ +"""Resource lookup memoizes package directory listings without going stale.""" +import os +import tempfile +import time +import unittest +from pathlib import Path +from unittest import mock + +from ovos_workshop import resource_files +from ovos_workshop.resource_files import (ResourceType, match_lang_directories, + locate_lang_directories) + + +def _touch(path: Path, text: str = "x\n"): + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + +class TestResourceLookupCache(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.TemporaryDirectory() + self.skill = Path(self.tmp.name) + for lang in ("en-US", "fr-FR", "de-DE", "es-ES"): + _touch(self.skill / "locale" / lang / "intents" / "hello.intent") + resource_files._subdir_cache.clear() + resource_files._file_index_cache.clear() + + def tearDown(self): + self.tmp.cleanup() + + def test_same_result_as_uncached(self): + self.assertEqual(locate_lang_directories("fr-FR", str(self.skill)), + [self.skill / "locale" / "fr-FR"]) + self.assertEqual(locate_lang_directories("en-US", str(self.skill)), + [self.skill / "locale" / "en-US"]) + + def test_listing_is_reused(self): + locate_lang_directories("fr-FR", str(self.skill)) + with mock.patch.object(resource_files.os, "scandir", + side_effect=AssertionError("relisted")): + for lang in ("fr-FR", "de-DE", "en-US"): + locate_lang_directories(lang, str(self.skill)) + + def test_new_language_directory_is_seen(self): + self.assertEqual(locate_lang_directories("it-IT", str(self.skill)), []) + time.sleep(0.01) + _touch(self.skill / "locale" / "it-IT" / "intents" / "hello.intent") + # the locale directory's mtime moved, so the listing is refreshed + self.assertEqual(locate_lang_directories("it-IT", str(self.skill)), + [self.skill / "locale" / "it-IT"]) + + def test_resource_file_located_once_per_directory(self): + rt = ResourceType("intent", ".intent", "fr-FR") + rt.locate_base_directory(str(self.skill)) + first = resource_files.IntentFile(rt, "hello").file_path + self.assertEqual(first, self.skill / "locale" / "fr-FR" / "intents" / "hello.intent") + with mock.patch.object(resource_files, "walk", + side_effect=AssertionError("walked again")): + again = resource_files.IntentFile(rt, "hello.intent").file_path + missing = resource_files.IntentFile(rt, "nope").file_path + self.assertEqual(again, first) + self.assertIsNone(missing) + + def test_missing_directory(self): + self.assertEqual(match_lang_directories("en-US", [self.skill / "absent"]), []) + + +if __name__ == "__main__": + unittest.main() From e5d15ae5a5d61620560cde45501683bb1e83c82e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Trellu?= Date: Mon, 12 Oct 2026 08:47:23 -0400 Subject: [PATCH 2/3] perf: read a language's vocabularies once, not once per intent file SkillResources._locale_vocabularies() walked the vocabulary tree and read and expanded every .voc file again for each .intent and .blacklist the skill loaded; on an 11-language runtime it was the largest single cost left in skill loading (py-spy). The map is now memoized per (directory, inode, mtime) of the package directories and handed out as a copy; when a user override directory exists nothing is cached, since that one can change at runtime. Co-Authored-By: Claude Opus 5.5 --- ovos_workshop/resource_files.py | 23 +++++++++++++- test/unittests/test_resource_lookup_cache.py | 32 ++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/ovos_workshop/resource_files.py b/ovos_workshop/resource_files.py index 9b1b969a..7e8124a4 100644 --- a/ovos_workshop/resource_files.py +++ b/ovos_workshop/resource_files.py @@ -880,8 +880,29 @@ def _locale_vocabularies(self) -> Dict[str, List[str]]: vocabulary of that name. User overrides take precedence over the skill's own resources, which take precedence over workshop-shipped resources. """ - vocabularies: Dict[str, List[str]] = {} + # Called for every .intent and .blacklist file the skill loads, and + # each call walked the vocabulary tree and read and expanded every + # .voc in it again. Package directories do not change while + # installed, so the map is kept per directory identity; a user + # override directory (editable at runtime) is never cached. vocab_type = self.types.vocabulary + memo_key = None + if not vocab_type.user_directory: + memo_key = tuple((str(base), _dir_identity(base)) + for base in (vocab_type.base_directory, + vocab_type.workshop_directory) + if base) + memo = getattr(self, "_vocabularies_memo", None) + if memo is not None and memo[0] == memo_key: + return {name: list(members) for name, members in memo[1].items()} + vocabularies = self._read_locale_vocabularies(vocab_type) + if memo_key is not None: + self._vocabularies_memo = ( + memo_key, {name: list(members) for name, members in vocabularies.items()}) + return vocabularies + + def _read_locale_vocabularies(self, vocab_type) -> Dict[str, List[str]]: + vocabularies: Dict[str, List[str]] = {} for base in (vocab_type.user_directory, vocab_type.base_directory, vocab_type.workshop_directory): diff --git a/test/unittests/test_resource_lookup_cache.py b/test/unittests/test_resource_lookup_cache.py index 76ca7699..67d95a02 100644 --- a/test/unittests/test_resource_lookup_cache.py +++ b/test/unittests/test_resource_lookup_cache.py @@ -67,3 +67,35 @@ def test_missing_directory(self): if __name__ == "__main__": unittest.main() + + +class TestLocaleVocabulariesMemo(unittest.TestCase): + def setUp(self): + from ovos_workshop.resource_files import SkillResources + self.tmp = tempfile.TemporaryDirectory() + self.skill = Path(self.tmp.name) + _touch(self.skill / "locale" / "en-US" / "vocab" / "colour.voc", "Red\nblue\n") + _touch(self.skill / "locale" / "en-US" / "intents" / "paint.intent", "paint it \n") + resource_files._subdir_cache.clear() + resource_files._file_index_cache.clear() + self.resources = SkillResources(str(self.skill), "en-US", skill_id="test.skill") + + def tearDown(self): + self.tmp.cleanup() + + def test_read_once_and_copied(self): + first = self.resources._locale_vocabularies() + self.assertEqual(first["colour"], ["red", "blue"]) # workshop's own .voc ride along + expected = {name: list(members) for name, members in first.items()} + first["colour"].append("mutated") + with mock.patch.object(resource_files, "walk", + side_effect=AssertionError("walked again")): + second = self.resources._locale_vocabularies() + self.assertEqual(second, expected) + + def test_user_override_directory_is_not_cached(self): + self.resources._locale_vocabularies() + self.resources.types.vocabulary.user_directory = self.skill / "locale" / "en-US" + with mock.patch.object(resource_files, "walk", wraps=resource_files.walk) as spy: + self.resources._locale_vocabularies() + self.assertTrue(spy.called) From 6e8b177f02d45b7fee8fe3e62267a1e29782f868 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ga=C3=ABtan=20Trellu?= Date: Mon, 12 Oct 2026 09:39:42 -0400 Subject: [PATCH 3/3] fix: validate the recursive resource caches against the whole tree The file index and the vocabulary memo were reused while the root directory's identity was unchanged, but a directory's mtime only moves for entries added, removed or renamed in that directory itself: a file added in a subdirectory, or a .voc edited in place, left them stale. Both caches now record the identity of every directory they walked, and the vocabulary memo also each .voc it read (inode, mtime, size); a cached result is reused only while all of them still match, which costs one stat per entry instead of a walk and a re-read. Tests cover a file added in a subdirectory and a vocabulary edited in place. Co-Authored-By: Claude Opus 5.5 --- ovos_workshop/resource_files.py | 89 +++++++++++++++----- test/unittests/test_resource_lookup_cache.py | 46 ++++++++++ 2 files changed, 113 insertions(+), 22 deletions(-) diff --git a/ovos_workshop/resource_files.py b/ovos_workshop/resource_files.py index 7e8124a4..d67f4e26 100644 --- a/ovos_workshop/resource_files.py +++ b/ovos_workshop/resource_files.py @@ -116,7 +116,7 @@ def match_lang_directories(lang: str, base_dirs: List[Path]) -> List[Path]: # new mtime), so both are memoized on (path, inode, mtime). _DIR_CACHE_MAX = 4096 _subdir_cache: Dict[str, Tuple[Tuple[int, int], Tuple[str, ...]]] = {} -_file_index_cache: Dict[str, Tuple[Tuple[int, int], Dict[str, Path]]] = {} +_file_index_cache: Dict[str, Tuple[tuple, Dict[str, Path]]] = {} def _dir_identity(directory) -> Optional[Tuple[int, int]]: @@ -155,26 +155,54 @@ def _lang_dir_distance(lang: str, name: str) -> Optional[float]: return lang_distance(lang, name) +def _tree_still_matches(signature) -> bool: + """True when every directory and file recorded in `signature` still has + the identity it had when the cached result was built. + + A directory's mtime moves when an entry is added, removed or renamed in + it -- but only in it, not in its subdirectories -- so a recursive cache + records every directory it walked, and every file whose content it used. + """ + for kind, path, identity in signature: + try: + st = os.stat(path) + except OSError: + return False + if kind == "d": + current = (st.st_ino, st.st_mtime_ns) + else: + current = (st.st_ino, st.st_mtime_ns, st.st_size) + if current != identity: + return False + return True + + def _walk_file_index(directory) -> Dict[str, Path]: """file name -> path for every file under `directory`, memoized. Reproduces what ResourceFile._locate's walk returned: when several subdirectories hold the same file name, the one the walk reaches last. + The cached index is reused only while every directory the walk visited + is unchanged, so a file added anywhere in the tree is seen. """ key = str(directory) - identity = _dir_identity(key) - if identity is None: - return {} cached = _file_index_cache.get(key) - if cached is not None and cached[0] == identity: + if cached is not None and _tree_still_matches(cached[0]): return cached[1] index: Dict[str, Path] = {} + signature = [] for root, _, file_names in walk(key): + identity = _dir_identity(root) + if identity is None: + continue + signature.append(("d", root, identity)) for file_name in file_names: index[file_name] = Path(root, file_name) + if not signature: + return {} if len(_file_index_cache) >= _DIR_CACHE_MAX: _file_index_cache.clear() - _file_index_cache[key] = (identity, index) + _file_index_cache[key] = (tuple(signature), index) return index @@ -882,26 +910,31 @@ def _locale_vocabularies(self) -> Dict[str, List[str]]: """ # Called for every .intent and .blacklist file the skill loads, and # each call walked the vocabulary tree and read and expanded every - # .voc in it again. Package directories do not change while - # installed, so the map is kept per directory identity; a user - # override directory (editable at runtime) is never cached. + # .voc in it again. The map is reused while every directory walked + # and every .voc read still has the identity it had (one stat each, + # no reads); a user override directory is never cached. vocab_type = self.types.vocabulary - memo_key = None - if not vocab_type.user_directory: - memo_key = tuple((str(base), _dir_identity(base)) - for base in (vocab_type.base_directory, - vocab_type.workshop_directory) - if base) + cacheable = not vocab_type.user_directory + if cacheable: memo = getattr(self, "_vocabularies_memo", None) - if memo is not None and memo[0] == memo_key: - return {name: list(members) for name, members in memo[1].items()} - vocabularies = self._read_locale_vocabularies(vocab_type) - if memo_key is not None: + if memo is not None and memo[0] == (vocab_type.base_directory, + vocab_type.workshop_directory) \ + and _tree_still_matches(memo[1]): + return {name: list(members) for name, members in memo[2].items()} + signature: list = [] + vocabularies = self._read_locale_vocabularies(vocab_type, signature) + if cacheable: self._vocabularies_memo = ( - memo_key, {name: list(members) for name, members in vocabularies.items()}) + (vocab_type.base_directory, vocab_type.workshop_directory), + tuple(signature), + {name: list(members) for name, members in vocabularies.items()}) return vocabularies - def _read_locale_vocabularies(self, vocab_type) -> Dict[str, List[str]]: + def _read_locale_vocabularies(self, vocab_type, + signature: Optional[list] = None + ) -> Dict[str, List[str]]: + """Read every .voc in the vocabulary tree; when `signature` is given, + append the identity of each directory walked and each file read.""" vocabularies: Dict[str, List[str]] = {} for base in (vocab_type.user_directory, vocab_type.base_directory, @@ -909,6 +942,10 @@ def _read_locale_vocabularies(self, vocab_type) -> Dict[str, List[str]]: if not base: continue for directory, _, files in walk(str(base)): + if signature is not None: + identity = _dir_identity(directory) + if identity is not None: + signature.append(("d", directory, identity)) for file_name in files: if not file_name.endswith(".voc"): continue @@ -916,7 +953,15 @@ def _read_locale_vocabularies(self, vocab_type) -> Dict[str, List[str]]: if name in vocabularies: continue members: List[str] = [] - with open(Path(directory, file_name)) as voc_file: + voc_path = Path(directory, file_name) + if signature is not None: + try: + st = os.stat(voc_path) + signature.append(("f", str(voc_path), + (st.st_ino, st.st_mtime_ns, st.st_size))) + except OSError: + pass + with open(voc_path) as voc_file: for line in (ln.strip().lower() for ln in voc_file): if line and not line.startswith("#"): # a .voc line MAY hold comma/parenthesis diff --git a/test/unittests/test_resource_lookup_cache.py b/test/unittests/test_resource_lookup_cache.py index 67d95a02..c025daad 100644 --- a/test/unittests/test_resource_lookup_cache.py +++ b/test/unittests/test_resource_lookup_cache.py @@ -99,3 +99,49 @@ def test_user_override_directory_is_not_cached(self): with mock.patch.object(resource_files, "walk", wraps=resource_files.walk) as spy: self.resources._locale_vocabularies() self.assertTrue(spy.called) + + +class TestCachesSeeChangesDeepInTheTree(unittest.TestCase): + """A cache validated by the root's identity alone would miss these.""" + + def setUp(self): + from ovos_workshop.resource_files import SkillResources + self.tmp = tempfile.TemporaryDirectory() + self.skill = Path(self.tmp.name) + _touch(self.skill / "locale" / "en-US" / "vocab" / "colour.voc", "red\n") + _touch(self.skill / "locale" / "en-US" / "intents" / "paint.intent", "paint it \n") + resource_files._subdir_cache.clear() + resource_files._file_index_cache.clear() + self.resources = SkillResources(str(self.skill), "en-US", skill_id="test.skill") + + def tearDown(self): + self.tmp.cleanup() + + def test_file_added_in_a_subdirectory_is_found(self): + rt = ResourceType("intent", ".intent", "en-US") + rt.locate_base_directory(str(self.skill)) + self.assertIsNone(resource_files.IntentFile(rt, "erase").file_path) + root_before = os.stat(self.skill / "locale" / "en-US").st_mtime_ns + time.sleep(0.01) + _touch(self.skill / "locale" / "en-US" / "intents" / "erase.intent", "erase it\n") + # the language directory itself did not change... + self.assertEqual(os.stat(self.skill / "locale" / "en-US").st_mtime_ns, root_before) + # ...but the subdirectory did, and the index notices + self.assertEqual(resource_files.IntentFile(rt, "erase").file_path, + self.skill / "locale" / "en-US" / "intents" / "erase.intent") + + def test_vocabulary_added_in_a_subdirectory_is_seen(self): + self.assertNotIn("size", self.resources._locale_vocabularies()) + time.sleep(0.01) + _touch(self.skill / "locale" / "en-US" / "vocab" / "size.voc", "big\n") + self.assertEqual(self.resources._locale_vocabularies()["size"], ["big"]) + + def test_vocabulary_edited_in_place_is_reread(self): + self.assertEqual(self.resources._locale_vocabularies()["colour"], ["red"]) + voc = self.skill / "locale" / "en-US" / "vocab" / "colour.voc" + dir_before = os.stat(voc.parent).st_mtime_ns + time.sleep(0.01) + with open(voc, "w") as f: # same inode, same directory entry + f.write("red\ngreen\n") + self.assertEqual(os.stat(voc.parent).st_mtime_ns, dir_before) + self.assertEqual(self.resources._locale_vocabularies()["colour"], ["red", "green"])