mirror of
https://github.com/tiennm99/serena.git
synced 2026-10-12 14:15:14 +00:00
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 <noreply@anthropic.com>
Co-Authored-By: Dominik Jain <dominik.jain@oraios-ai.de>
This commit is contained in:
1 parent
c6fbd1c593
commit
f0693a10c1
3 files changed
+31
-13
No files matched your search
@@ -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
|
||||
|
||||
+21
-10
@@ -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:
|
||||
|
||||
@@ -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)
|
||||
Reference in new issue
Block a user