Improve performance by skipping unnecessary normalisation #3751
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This speeds up black by about 40% when the cache is full
| ) -> bool: | ||
| path = root / root_relative_path | ||
| # Note that this logic is sensitive to the ordering of gitignore_dict. Callers must |
There was a problem hiding this comment.
Is it? Looks like we check all entries either way.
There was a problem hiding this comment.
I might be being really dumb, but I think it is, because of the break on L285 / L292. You can hit that in the case of symlinks.
I also think it's maybe a little questionable to be applying gitignore to the resolved file rather than the symlink when formatting the symlink (e.g. a gitignored symlink can now affect what gets formatted), but whatever.
There was a problem hiding this comment.
OK I stared at the code some more and this does make sense now. (I thought it mattered only for broken symlinks, but that's not the case.)
We pass a dict here that gets built separately for every directory, since we look for .gitignore in every directory, so given a directory structure root/a/b/c.py, the dict will contain root/.gitignore, root/a/.gitignore, root/a/b/.gitignore in that order. The break happens for the first path where the symlink points outside the directory, so it makes sense to ignore all subsequent gitignores as they definitely won't match either.
Agree that it probably makes more sense to apply gitignore before resolving symlinks, but that's something do in a separate PR.
JelleZijlstra
approved these changes
Jul 9, 2023
| ) -> bool: | ||
| path = root / root_relative_path | ||
| # Note that this logic is sensitive to the ordering of gitignore_dict. Callers must |
There was a problem hiding this comment.
OK I stared at the code some more and this does make sense now. (I thought it mattered only for broken symlinks, but that's not the case.)
We pass a dict here that gets built separately for every directory, since we look for .gitignore in every directory, so given a directory structure root/a/b/c.py, the dict will contain root/.gitignore, root/a/.gitignore, root/a/b/.gitignore in that order. The break happens for the first path where the symlink points outside the directory, so it makes sense to ignore all subsequent gitignores as they definitely won't match either.
Agree that it probably makes more sense to apply gitignore before resolving symlinks, but that's something do in a separate PR.
hauntsaninja
added a commit
to hauntsaninja/black
that referenced
this pull request
Feb 11, 2024
This relates to psf#4015, psf#4161 and the behaviour of os.getcwd() Black is a big user of pathlib and as such loves doing `.resolve()`, since for a long time it was the only good way of getting an absolute path in pathlib. However, this has two problems: The first minor problem is performance, e.g. in psf#3751 I (safely) got rid of a bunch of `.resolve()` which made Black 40% faster on cached runs. The second more important problem is that always resolving symlinks results in unintuitive exclusion behaviour. For instance, a gitignored symlink should never alter formatting of your actual code. This was reported by users a few times. In psf#3846, I improved the exclusion rule logic for symlinks in `gen_python_files` and everything was good. But `gen_python_files` isn't enough, there's also `get_sources`, which handles user specified paths directly (instead of files Black discovers). So in psf#4015, I made a very similar change to psf#3846 for `get_sources`, and this is where some problems began. The core issue was the line: ``` root_relative_path = path.absolute().relative_to(root).as_posix() ``` The first issue is that despite root being computed from user inputs, we call `.resolve()` while computing it (likely unecessarily). Which means that `path` may not actually be relative to `root`. So I started off this PR trying to fix that, when I ran into the second issue. Which is that `os.getcwd()` (as called by `os.path.abspath` or `Path.absolute` or `Path.cwd`) also often resolves symlinks! ``` >>> import os >>> os.environ.get("PWD") '/Users/shantanu/dev/black/symlink/bug' >>> os.getcwd() '/Users/shantanu/dev/black/actual/bug' ``` This also meant that the breakage often would not show up when input relative paths. This doesn't affect `gen_python_files` / psf#3846 because things are always absolute and known to be relative to `root`. Anyway, it looks like psf#4161 fixed the crash by just swallowing the error and ignoring the file. Instead, we should just try to compute the actual relative path. I think this PR should be quite safe, but we could also consider reverting some of the previous changes; the associated issues weren't too popular. At the same time, I think there's still behaviour that can be improved and I kind of want to make larger changes, but maybe I'll save that for if we do something like psf#3952 Hopefully fixes psf#4205, fixes psf#4209, actual fix for psf#4077
hauntsaninja
added a commit
to hauntsaninja/black
that referenced
this pull request
Feb 11, 2024
This relates to psf#4015, psf#4161 and the behaviour of os.getcwd() Black is a big user of pathlib and as such loves doing `.resolve()`, since for a long time it was the only good way of getting an absolute path in pathlib. However, this has two problems: The first minor problem is performance, e.g. in psf#3751 I (safely) got rid of a bunch of `.resolve()` which made Black 40% faster on cached runs. The second more important problem is that always resolving symlinks results in unintuitive exclusion behaviour. For instance, a gitignored symlink should never alter formatting of your actual code. This kind of thing was reported by users a few times. In psf#3846, I improved the exclusion rule logic for symlinks in `gen_python_files` and everything was good. But `gen_python_files` isn't enough, there's also `get_sources`, which handles user specified paths directly (instead of files Black discovers). So in psf#4015, I made a very similar change to psf#3846 for `get_sources`, and this is where some problems began. The core issue was the line: ``` root_relative_path = path.absolute().relative_to(root).as_posix() ``` The first issue is that despite root being computed from user inputs, we call `.resolve()` while computing it (likely unecessarily). Which means that `path` may not actually be relative to `root`. So I started off this PR trying to fix that, when I ran into the second issue. Which is that `os.getcwd()` (as called by `os.path.abspath` or `Path.absolute` or `Path.cwd`) also often resolves symlinks! ``` >>> import os >>> os.environ.get("PWD") '/Users/shantanu/dev/black/symlink/bug' >>> os.getcwd() '/Users/shantanu/dev/black/actual/bug' ``` This also meant that the breakage often would not show up when input relative paths. This doesn't affect `gen_python_files` / psf#3846 because things are always absolute and known to be relative to `root`. Anyway, it looks like psf#4161 fixed the crash by just swallowing the error and ignoring the file. Instead, we should just try to compute the actual relative path. I think this PR should be quite safe, but we could also consider reverting some of the previous changes; the associated issues weren't too popular. At the same time, I think there's still behaviour that can be improved and I kind of want to make larger changes, but maybe I'll save that for if we do something like psf#3952 Hopefully fixes psf#4205, fixes psf#4209, actual fix for psf#4077
hauntsaninja
added a commit
that referenced
this pull request
Feb 12, 2024
This relates to #4015, #4161 and the behaviour of os.getcwd() Black is a big user of pathlib and as such loves doing `.resolve()`, since for a long time it was the only good way of getting an absolute path in pathlib. However, this has two problems: The first minor problem is performance, e.g. in #3751 I (safely) got rid of a bunch of `.resolve()` which made Black 40% faster on cached runs. The second more important problem is that always resolving symlinks results in unintuitive exclusion behaviour. For instance, a gitignored symlink should never alter formatting of your actual code. This kind of thing was reported by users a few times. In #3846, I improved the exclusion rule logic for symlinks in `gen_python_files` and everything was good. But `gen_python_files` isn't enough, there's also `get_sources`, which handles user specified paths directly (instead of files Black discovers). So in #4015, I made a very similar change to #3846 for `get_sources`, and this is where some problems began. The core issue was the line: ``` root_relative_path = path.absolute().relative_to(root).as_posix() ``` The first issue is that despite root being computed from user inputs, we call `.resolve()` while computing it (likely unecessarily). Which means that `path` may not actually be relative to `root`. So I started off this PR trying to fix that, when I ran into the second issue. Which is that `os.getcwd()` (as called by `os.path.abspath` or `Path.absolute` or `Path.cwd`) also often resolves symlinks! ``` >>> import os >>> os.environ.get("PWD") '/Users/shantanu/dev/black/symlink/bug' >>> os.getcwd() '/Users/shantanu/dev/black/actual/bug' ``` This also meant that the breakage often would not show up when input relative paths. This doesn't affect `gen_python_files` / #3846 because things are always absolute and known to be relative to `root`. Anyway, it looks like #4161 fixed the crash by just swallowing the error and ignoring the file. Instead, we should just try to compute the actual relative path. I think this PR should be quite safe, but we could also consider reverting some of the previous changes; the associated issues weren't too popular. At the same time, I think there's still behaviour that can be improved and I kind of want to make larger changes, but maybe I'll save that for if we do something like #3952 Hopefully fixes #4205, fixes #4209, actual fix for #4077
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This speeds up black by about 40% on my repo at work when the cache is full. This change is intended to exactly preserve behaviour (other than a slight improvement to verbose logging).