From f0693a10c12dc897dc28e87347abe0bd2bcc167c Mon Sep 17 00:00:00 2001 From: Volksie Date: Fri, 18 Sep 2026 10:00:44 -0600 Subject: [PATCH] perf: let a directory traversal tell is_ignored_path what it already knows Project.gather_source_files walks the tree with os.walk, which hands back directories and files separately, and then calls is_ignored_path on each one. That method re-derived file-ness from the filesystem: an os.path.exists and an os.path.isfile in _is_ignored_relative_path, plus an os.path.isdir in match_path. Three syscalls per path, for an answer the caller already had. is_ignored_path, _is_ignored_relative_path and match_path now accept an optional hint, and gather_source_files supplies it from the os.walk split. The parameter defaults to None, which determines file-ness from the filesystem exactly as before, so no existing caller changes behaviour. Measured on a repository with 97,549 tracked source files out of 707,889 total (Unreal Engine source plus three game projects; Windows 11, Python 3.13): gather_source_files() 67.8s -> 11.6s The returned file list is byte-identical before and after (sha256 over the sorted relative paths, 10,390,077 bytes). For context on where the time went: a bare os.walk of the whole 708k-file tree takes 10.7s, so this was never I/O-bound. cProfile over 15,000 real is_ignored_path calls attributed 3.41s to nt._path_exists, nt._path_isfile and nt._path_isdir - about 82% of the per-call cost. The new test asserts equivalence rather than specific verdicts: for every path in a fixture tree, across several ignore configurations, the hinted call must agree with the unhinted one. It covers the two cases where guessing file-ness from the name would go wrong - a directory with a suffix, and an extensionless file - and was checked against a deliberate inversion of the hint, which it catches. Refs #2077 Co-Authored-By: Claude Opus 5 Co-Authored-By: Dominik Jain --- CHANGELOG.md | 3 +++ src/serena/project.py | 31 +++++++++++++++++++++---------- src/serena/util/file_system.py | 10 +++++++--- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb089720..ea1271b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,6 +38,9 @@ Status of the `main` branch. Changes prior to the next official version change w MCP session, since the MCP SDK v2 no longer provides session identifiers and client session usage was inconsistent anyway. Tools that need a session id (e.g. `activate_project`, the REPL tool) now take it as an explicit parameter, obtained from `initial_instructions` + - Performance: `Project.gather_source_files` transitively re-derived from the filesystem, for every path, + whether that path was a file or a directory; related methods/functions now receive the information + as a parameter where it is already known (#2077) * CLI: - Fix: `project health-check` reported `Health check passed - All tools working correctly` and diff --git a/src/serena/project.py b/src/serena/project.py index 396a01cf..5f2570e5 100644 --- a/src/serena/project.py +++ b/src/serena/project.py @@ -212,7 +212,9 @@ class Project(ToStringMixin): ) return self.__ignored_patterns - def _is_ignored_relative_path(self, relative_path: str | Path, ignore_non_source_files: bool = True) -> bool: + def _is_ignored_relative_path( + self, relative_path: str | Path, ignore_non_source_files: bool = True, is_file: bool | None = None + ) -> bool: """ Determine whether a path should be ignored based on file type and ignore patterns. Returns False for non-existent paths since they cannot be matched by ignore patterns. @@ -220,6 +222,7 @@ class Project(ToStringMixin): :param relative_path: Relative path to check :param ignore_non_source_files: whether files that are not source files (according to the file masks determined by the project's programming language) shall be ignored + :param is_file: whether the path exists and is a file, for callers that already know :return: whether the path should be ignored """ @@ -230,13 +233,16 @@ class Project(ToStringMixin): return False abs_path = os.path.join(self.project_root, relative_path) - if not os.path.exists(abs_path): - log.debug(f"Path {abs_path} does not exist, skipping ignore check") - return False + if is_file is None: + if not os.path.exists(abs_path): + log.debug(f"Path {abs_path} does not exist, skipping ignore check") + return False # check code file restriction (depending on backend) if ignore_non_source_files: - if os.path.isfile(abs_path): + if is_file is None: + is_file = os.path.isfile(abs_path) + if is_file: # non-source files are ignored if not self.language_backend.is_source_file(abs_path, self): return True @@ -248,15 +254,18 @@ class Project(ToStringMixin): if len(rel_path.parts) > 0 and ".git" in rel_path.parts: return True - return match_path(str(relative_path), self._ignore_spec, root_path=self.project_root) + is_dir = None if is_file is None else not is_file + return match_path(str(relative_path), self._ignore_spec, root_path=self.project_root, is_dir=is_dir) - def is_ignored_path(self, path: str | Path, ignore_non_source_files: bool = False) -> bool: + def is_ignored_path(self, path: str | Path, ignore_non_source_files: bool = False, is_file: bool | None = None) -> bool: """ Checks whether the given path is ignored :param path: the path to check, can be absolute or relative :param ignore_non_source_files: whether to ignore files that are not source files (according to the file masks determined by the project's programming language) + :param is_file: whether the path exists and is a file, for callers that already know; + see :meth:`_is_ignored_relative_path`. `None` determines it from the filesystem. """ path = Path(path) if path.is_absolute(): @@ -270,7 +279,7 @@ class Project(ToStringMixin): else: relative_path = path - return self._is_ignored_relative_path(str(relative_path), ignore_non_source_files=ignore_non_source_files) + return self._is_ignored_relative_path(str(relative_path), ignore_non_source_files=ignore_non_source_files, is_file=is_file) def get_is_ignored_path_fn(self, base_path: str, skip_ignored_paths: bool) -> Callable[[str], bool]: """ @@ -350,15 +359,17 @@ class Project(ToStringMixin): if os.path.isfile(start_path): return [relative_path] else: + # os.walk hands back directories and files separately, so `is_file` is already known here and + # does not have to be re-derived from the filesystem for every one of them. for root, dirs, files in os.walk(start_path, followlinks=True): # prevent recursion into ignored directories - dirs[:] = [d for d in dirs if not self.is_ignored_path(os.path.join(root, d))] + dirs[:] = [d for d in dirs if not self.is_ignored_path(os.path.join(root, d), is_file=False)] # collect non-ignored files for file in files: abs_file_path = os.path.join(root, file) try: - if not self.is_ignored_path(abs_file_path, ignore_non_source_files=True): + if not self.is_ignored_path(abs_file_path, ignore_non_source_files=True, is_file=True): try: rel_file_path = os.path.relpath(abs_file_path, start=self.project_root) except Exception: diff --git a/src/serena/util/file_system.py b/src/serena/util/file_system.py index b08d483b..b78af51f 100644 --- a/src/serena/util/file_system.py +++ b/src/serena/util/file_system.py @@ -440,7 +440,7 @@ class GitignoreParser: self._load_gitignore_files() -def match_path(relative_path: str, path_spec: PathSpec, root_path: str = "") -> bool: +def match_path(relative_path: str, path_spec: PathSpec, root_path: str = "", is_dir: bool | None = None) -> bool: """ Match a relative path against a given pathspec. Just pathspec.match_file() is not enough, we need to do some massaging to fix issues with pathspec matching. @@ -448,6 +448,8 @@ def match_path(relative_path: str, path_spec: PathSpec, root_path: str = "") -> :param relative_path: relative path to match against the pathspec :param path_spec: the pathspec to match against :param root_path: the root path from which the relative path is derived + :param is_dir: whether the path is a directory, where the caller already knows; passing it avoids + an `os.path.isdir` call. `None` determines it from the filesystem. :return: """ if str(relative_path) in {"", "."}: @@ -465,7 +467,9 @@ def match_path(relative_path: str, path_spec: PathSpec, root_path: str = "") -> # pathspec can't handle the matching of directories if they don't end with a slash! # see https://github.com/cpburnz/python-pathspec/issues/89 - abs_path = os.path.abspath(os.path.join(root_path, relative_path)) - if os.path.isdir(abs_path) and not normalized_path.endswith("/"): + if is_dir is None: + abs_path = os.path.abspath(os.path.join(root_path, relative_path)) + is_dir = os.path.isdir(abs_path) + if is_dir and not normalized_path.endswith("/"): normalized_path = normalized_path + "/" return path_spec.match_file(normalized_path)