perf(packages): resolve file paths without pathlib containment - #19857
perf(packages): resolve file paths without pathlib containment#19857P403n1x87 wants to merge 1 commit into
Conversation
`filename_to_package` and the `is_user_code` / `is_third_party` / `is_stdlib`
predicates built on it are called per stack frame by the profiler and Dynamic
Instrumentation. A production alloc-size profile attributed a large share of
allocations to them, with `PurePath._str_normcase` and `Path.__new__` at the top
of the flame graph.
The cause is `PurePath.relative_to` / `is_relative_to`. On Python 3.12 a single
call allocates path objects and performs case-folded comparisons proportional to
the depth of *both* operands, whether it succeeds or fails. `packages.py` probed
one filename against every `sys.path` entry across three separate fallback
helpers, so almost all of that work was thrown away on the miss path.
Add `ddtrace/internal/utils/paths.py`, which answers the same questions by
comparing the `PurePath.parts` pathlib has already parsed: containment becomes a
prefix check over a tuple of strings, with no intermediate objects and no
exceptions when the answer is no. `relative_parts`, `is_contained`,
`relative_path` and `deepest_containing_root` replace the ad-hoc probing here and
at eleven call sites in `coverage/`, `ci_visibility/`, `testing/` and
`debugging/` that were open-coding the same patterns -- several of them paying
for containment twice by calling `is_relative_to` and then `relative_to`.
Comparing parsed components rather than strings is also what makes the anchor
handling correct: `/` and POSIX's separate `//`, or `C:` and `C:\`, no longer
match each other, and normalization is guaranteed by `PurePath` construction
rather than assumed.
Also cut redundant filesystem work the profile exposed:
* `_relative_to_known_root` and `_root_module` each called `path.resolve()` on
the same path. It is now resolved once in `filename_to_package` and passed
alongside the original, which the `sys.path` probes still need unresolved or
symlinked entries stop matching.
* `_is_install_root` and `_root_ships_distribution` walked the same directory
independently, the latter uncached on every probe. One cached
`_shipped_distributions` listing now serves both.
* The `__init__.py` probe behind `_effective_root` is cached on the package name
instead of being repeated per source file.
Measured on 3.12. `filename_to_package` on a cache miss, against the real
distribution mapping:
before 1198 us / 6634 B
after 284 us / 3138 B 4.2x faster, 53% less allocated
An Austin profile of the miss path (262k misses, 15.7 kHz, wall) confirms where
the remaining cost sits and that containment is no longer part of it:
Path.resolve() 89.3% of the miss path
parents-walk fallback 3.8%
containment helpers 1.3%
Removing the duplicate resolve took the miss from 57.1 us to 49.5 us, 13% end to
end, and the follow-up profile shows `resolve()` at a single call site with the
two helpers down from 23.4%/9.5% to 2.1%/1.6%.
The helpers are verified against pathlib itself rather than by construction: an
exhaustive differential test over every path/root pair asserts they agree with
`relative_to`, on 3.9 through 3.14. Because CI here is POSIX-only, the Windows
rules are forced on and differentially tested too -- that is what caught a case
folding bug, since the lowercase of U+0130 is two characters and an earlier
offset-based implementation silently truncated components under a Turkish
directory name.
No behaviour change is intended. Two latent bugs are fixed incidentally: an
`IndexError` escaping `filename_to_package` when a path was itself a `sys.path`
entry, and the `PurePath.parents` walk in `_resolve_source_file` no longer being
quadratic in path depth.
P403n1x87
added
the
changelog/no-changelog
Circular import analysis
|
Dependency direction analysis
|
|
Codeowners resolved asResolved from the full PR diff against |
BenchmarksBenchmark execution time: 2026-08-25 15:19:09 Comparing candidate commit 59493e2 in PR branch Found 0 performance improvements and 11 performance regressions! Performance is the same for 606 metrics, 10 unstable metrics.
|
| two characters, and an offset-based version silently truncated components. | ||
| """ | ||
| if _CASE_INSENSITIVE: | ||
| return all(a.lower() == b.lower() for a, b in zip(path_parts, root_parts)) |
There was a problem hiding this comment.
Is it guaranteed that normcase and lower are the same? I have virtually zero knowledge in file systems but it sounds like the kind of thing where file systems can be ✨ surprising ✨
There was a problem hiding this comment.
Once things are split into parts then it should be the same. This is only relevant for Win anyway 😅
| root_parts = root.parts | ||
| n = len(root_parts) | ||
|
|
||
| if not n: |
There was a problem hiding this comment.
| if not n: | |
| if not root_parts: |
Would that make sense in the interest of making things clearer?
| root = parent / base | ||
| return base if root.is_dir() and (root / "__init__.py").exists() else "/".join(rel_path.parts[:2]) | ||
| return root.is_dir() and (root / "__init__.py").exists() |
There was a problem hiding this comment.
I guess we're ok with using Pathlib here because we'd have to pay the price of transforming the path to a string anyway in order to do the same check without it?
+ I guess it shouldn't be called too often?
There was a problem hiding this comment.
these pathlib operations are fine as they don't allocate memory for new objects
| """Normalize a distribution name for comparison (PEP 503-ish). | ||
|
|
||
| ``.dist-info`` / ``.egg-info`` directories escape the project name (dashes | ||
| become underscores), so fold ``-``, ``_`` and ``.`` to a single form and |
There was a problem hiding this comment.
| become underscores), so fold ``-``, ``_`` and ``.`` to a single form and | |
| become underscores), so fold -, _ and . to a single form and |
To make the thing less... ugly? Not sure what to do with those cases
| pass | ||
| relative = relative_parts(item_path, workspace_path) | ||
| if relative is not None: | ||
| # as_posix() of a relative path is its components joined by "/". |
There was a problem hiding this comment.
| # as_posix() of a relative path is its components joined by "/". |
I'd say remove this comment since we don't have as_posix anywhere anymore
| # Making a path relative only strips leading components, so the | ||
| # last one is unchanged. |
There was a problem hiding this comment.
Same here, I think this comment only makes sense when you see the diff, otherwise it's puzzling more than it's helpful
| try: | ||
| return f"/{Path(absolute_path).relative_to(relative_to)}" | ||
| except ValueError: | ||
| # Both sides go through Path, which is the normalization relative_path needs. |
There was a problem hiding this comment.
| # Both sides go through Path, which is the normalization relative_path needs. |
| """A path that *is* a sys.path entry must resolve to None, not raise. | ||
|
|
||
| Such a path is relative to the root by zero components, so the root-module | ||
| lookup has no first component to inspect. _effective_root used to index | ||
| parts[0] unguarded, raising IndexError -- which filename_to_package does not | ||
| catch (it only handles ValueError/OSError), so it escaped to the caller. | ||
| """ |
There was a problem hiding this comment.
| """A path that *is* a sys.path entry must resolve to None, not raise. | |
| Such a path is relative to the root by zero components, so the root-module | |
| lookup has no first component to inspect. _effective_root used to index | |
| parts[0] unguarded, raising IndexError -- which filename_to_package does not | |
| catch (it only handles ValueError/OSError), so it escaped to the caller. | |
| """ | |
| """A path that *is* a sys.path entry must resolve to None, not raise.""" |
I'd suggest getting rid of all the details in the test docstrings since they typically don't bring much useful info
There was a problem hiding this comment.
Are these changes necessary as part of the PR or could we split them into a follow-up PR for the purely CI Visibility / coverage-related changes?
There was a problem hiding this comment.
👍 we could separate the changes
2 participants
Description
filename_to_packageand theis_user_code/is_third_party/is_stdlibpredicates built on it are called per stack frame by the profiler and Dynamic Instrumentation. A production alloc-size profile attributed a large share of allocations to them, withPurePath._str_normcaseandPath.__new__at the top of the flame graph.The cause is
PurePath.relative_to/is_relative_to. On Python 3.12 a single call allocates path objects and performs case-folded comparisons proportional to the depth of both operands, whether it succeeds or fails.packages.pyprobed one filename against everysys.pathentry across three separate fallback helpers, so almost all of that work was thrown away on the miss path.Add
ddtrace/internal/utils/paths.py, which answers the same questions by comparing thePurePath.partspathlib has already parsed: containment becomes a prefix check over a tuple of strings, with no intermediate objects and no exceptions when the answer is no.relative_parts,is_contained,relative_pathanddeepest_containing_rootreplace the ad-hoc probing here and at eleven call sites incoverage/,ci_visibility/,testing/anddebugging/that were open-coding the same patterns -- several of them paying for containment twice by callingis_relative_toand thenrelative_to.Comparing parsed components rather than strings is also what makes the anchor handling correct:
/and POSIX's separate//, orC:andC:\, no longer match each other, and normalization is guaranteed byPurePathconstruction rather than assumed.Also cut redundant filesystem work the profile exposed:
_relative_to_known_rootand_root_moduleeach calledpath.resolve()on the same path. It is now resolved once infilename_to_packageand passed alongside the original, which thesys.pathprobes still need unresolved or symlinked entries stop matching._is_install_rootand_root_ships_distributionwalked the same directory independently, the latter uncached on every probe. One cached_shipped_distributionslisting now serves both.__init__.pyprobe behind_effective_rootis cached on the package name instead of being repeated per source file.Measured on 3.12.
filename_to_packageon a cache miss, against the real distribution mapping:An Austin profile of the miss path (262k misses, 15.7 kHz, wall) confirms where the remaining cost sits and that containment is no longer part of it:
Removing the duplicate resolve took the miss from 57.1 us to 49.5 us, 13% end to end, and the follow-up profile shows
resolve()at a single call site with the two helpers down from 23.4%/9.5% to 2.1%/1.6%.The helpers are verified against pathlib itself rather than by construction: an exhaustive differential test over every path/root pair asserts they agree with
relative_to, on 3.9 through 3.14. Because CI here is POSIX-only, the Windows rules are forced on and differentially tested too -- that is what caught a case folding bug, since the lowercase of U+0130 is two characters and an earlier offset-based implementation silently truncated components under a Turkish directory name.No behaviour change is intended. Two latent bugs are fixed incidentally: an
IndexErrorescapingfilename_to_packagewhen a path was itself asys.pathentry, and thePurePath.parentswalk in_resolve_source_fileno longer being quadratic in path depth.