Optimize macOS reflink copies in gclient-new-workdir [chromium/tools/depot_tools : main]
Erik Staab (Gerrit)
Erik Staab added 1 comment![]( "Open in Gerrit")
return '-c' if sys.platform == 'darwin' else '--reflink'Note that there is a darwin check here to control the reflink flag. If we won't hit the darwin path here anymore maybe `--reflink` can be hardcoded on line 276.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu added 2 comments![]( "Open in Gerrit")
return '-c' if sys.platform == 'darwin' else '--reflink'Should we replace this with an `assert not(sys.platform == 'darwin')` since this method isn't hit on Mac anymore?
clonefile_darwin(root, workdir)Should we catch the error and try to fallback to a regular copy? That's documented the behavior of `cp -c` on Mac.
Related details
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 4 comments![]( "Open in Gerrit")
Note that there is a darwin check here to control the reflink flag. If we won't hit the darwin path here anymore maybe `--reflink` can be hardcoded on line 276.
Done
return '-c' if sys.platform == 'darwin' else '--reflink'Should we replace this with an `assert not(sys.platform == 'darwin')` since this method isn't hit on Mac anymore?
Done
clonefile_darwin(root, workdir)Should we catch the error and try to fallback to a regular copy? That's documented the behavior of `cp -c` on Mac.
Good idea. I fell back to cp -a -c, not just -c. Is that what you meant?
Related details
- Erik Staab
- Kirubel Aklilu
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
mark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
Related details
- Erik Staab
- Kirubel Aklilu
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kirubel Aklilu (Gerrit)
Kirubel Aklilu voted and added 1 comment![]( "Open in Gerrit")
Votes added by Kirubel Aklilu
| Code-Review | +1 |
1 comment
clonefile_darwin(root, workdir)Justin CohenShould we catch the error and try to fallback to a regular copy? That's documented the behavior of `cp -c` on Mac.
Good idea. I fell back to cp -a -c, not just -c. Is that what you meant?
- Erik Staab
- Justin Cohen
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 5 comments![]( "Open in Gerrit")
Does this ever need to copy things across filesystems?
['cp', '-a', cp_copy_on_write_flag(), src, dest],Can’t the `--reflink` be here now, in-line?
except (subprocess.CalledProcessError, OSError):I’d prefer to be explicit: the Apple one might raise an OSError, the subprocess one might raise a CalledProcessError, and there’s no overlap.
src_index = os.path.join(src_git_dir, 'index')
dest_index = os.path.join(dest_git_dir, 'index')
if sys.platform == 'darwin':
clonefile_darwin(src_index, dest_index)
else:
subprocess.check_call([
'cp',
'-a',
cp_copy_on_write_flag(),
src_index,
dest_index,
])Now that I see this happening a second time: you should really refactor this into common logic. There should be a function that knows how to copy(src, dest), and let it figure out whether to dispatch to the Apple function or start a cp subprocess.
I see a third and fourth follow, so, an even stronger justification.
except OSError:
subprocess.check_call(
['cp', '-a', '-c', root, workdir])?
Related details
- Erik Staab
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
mark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
mark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
Original:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_old-649c49853f28cbaec21bd80cf7df9cec.py /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
PYTHONPATH=/chrome/depot_tools python3 /slash/chrome/chrome 1.52s user 140.87s system 89% cpu 2:39.60 total
```
This is actually worse than using `--no-reflink`:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_old-649c49853f28cbaec21bd80cf7df9cec.py --no-reflink /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
37.77s user 57.30s system 81% cpu 1:56.20 total
```
Yours:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_new-a199c80e238c2ef2fa9df7d7f54c31d3.py /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
0.07s user 6.95s system 32% cpu 21.772 total
```
It then takes 1m14s to `rm -r`.
Additional thoughts:
I usually refer to /chrome, which is a symbolic link to /slash/chrome, but when I use /chrome/chrome.justin as the new_workdir, it totally messes up this script (both original and your modification): it detects copy-on write support, makes an empty directory at new_workdir, sticks a .gclient symbolic link in it, and then exits successfully with a cheerful message that it used copy-on-write. Total time 0.120s, but new_workdir doesn’t have anything other than .gclient in it.
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
Mark Mentovaimark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
mark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
Original:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_old-649c49853f28cbaec21bd80cf7df9cec.py /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
PYTHONPATH=/chrome/depot_tools python3 /slash/chrome/chrome 1.52s user 140.87s system 89% cpu 2:39.60 total
```This is actually worse than using `--no-reflink`:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_old-649c49853f28cbaec21bd80cf7df9cec.py --no-reflink /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
37.77s user 57.30s system 81% cpu 1:56.20 total```
Yours:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_new-a199c80e238c2ef2fa9df7d7f54c31d3.py /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
0.07s user 6.95s system 32% cpu 21.772 total
```It then takes 1m14s to `rm -r`.
Additional thoughts:
I usually refer to /chrome, which is a symbolic link to /slash/chrome, but when I use /chrome/chrome.justin as the new_workdir, it totally messes up this script (both original and your modification): it detects copy-on write support, makes an empty directory at new_workdir, sticks a .gclient symbolic link in it, and then exits successfully with a cheerful message that it used copy-on-write. Total time 0.120s, but new_workdir doesn’t have anything other than .gclient in it.
I usually refer to /chrome, which is a symbolic link to /slash/chrome, but when I use /chrome/chrome.justin as the new_workdir, it totally messes up this script (both original and your modification): it detects copy-on write support, makes an empty directory at new_workdir, sticks a .gclient symbolic link in it, and then exits successfully with a cheerful message that it used copy-on-write. Total time 0.120s, but new_workdir doesn’t have anything other than .gclient in it.
Probably due to the inconsistent application of `os.path.realpath` (ugh).
Related details
- Erik Staab
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 4 comments![]( "Open in Gerrit")
Can’t the `--reflink` be here now, in-line?
Done
I’d prefer to be explicit: the Apple one might raise an OSError, the subprocess one might raise a CalledProcessError, and there’s no overlap.
Done
src_index = os.path.join(src_git_dir, 'index')
dest_index = os.path.join(dest_git_dir, 'index')
if sys.platform == 'darwin':
clonefile_darwin(src_index, dest_index)
else:
subprocess.check_call([
'cp',
'-a',
cp_copy_on_write_flag(),
src_index,
dest_index,
])Now that I see this happening a second time: you should really refactor this into common logic. There should be a function that knows how to copy(src, dest), and let it figure out whether to dispatch to the Apple function or start a cp subprocess.
I see a third and fourth follow, so, an even stronger justification.
Done
except OSError:
subprocess.check_call(
['cp', '-a', '-c', root, workdir])Justin Cohen?
Kirubel requested we fall back to this if it fails.
Related details
- Erik Staab
- Mark Mentovai
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Does this ever need to copy things across filesystems?
I don't know. I'm sure someone will try to do that.
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
Mark Mentovaimark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
Mark Mentovaimark@ on my m1 macbook gclient-new-workdir goes from taking over 10 minutes to less than 2 minutes. I'm curious if it's faster on newer macbooks. Can you ptal?
Original:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_old-649c49853f28cbaec21bd80cf7df9cec.py /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
PYTHONPATH=/chrome/depot_tools python3 /slash/chrome/chrome 1.52s user 140.87s system 89% cpu 2:39.60 total
```This is actually worse than using `--no-reflink`:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_old-649c49853f28cbaec21bd80cf7df9cec.py --no-reflink /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
37.77s user 57.30s system 81% cpu 1:56.20 total```
Yours:
```
% time PYTHONPATH=/chrome/depot_tools python3 /tmp/gclient-new-workdir_new-a199c80e238c2ef2fa9df7d7f54c31d3.py /slash/chrome/chrome /slash/chrome/chrome.justin
[…]
0.07s user 6.95s system 32% cpu 21.772 total
```It then takes 1m14s to `rm -r`.
Additional thoughts:
I usually refer to /chrome, which is a symbolic link to /slash/chrome, but when I use /chrome/chrome.justin as the new_workdir, it totally messes up this script (both original and your modification): it detects copy-on write support, makes an empty directory at new_workdir, sticks a .gclient symbolic link in it, and then exits successfully with a cheerful message that it used copy-on-write. Total time 0.120s, but new_workdir doesn’t have anything other than .gclient in it.
I usually refer to /chrome, which is a symbolic link to /slash/chrome, but when I use /chrome/chrome.justin as the new_workdir, it totally messes up this script (both original and your modification): it detects copy-on write support, makes an empty directory at new_workdir, sticks a .gclient symbolic link in it, and then exits successfully with a cheerful message that it used copy-on-write. Total time 0.120s, but new_workdir doesn’t have anything other than .gclient in it.
Probably due to the inconsistent application of `os.path.realpath` (ugh).
I think patchset 6 will fix this. Can you try again?
Related details
- Erik Staab
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 7 comments![]( "Open in Gerrit")
libc_path = ctypes.util.find_library("c")This file seems to `'single-quote'` string literals. Stick with that for consistency.
res = _libc.clonefile(os.fsencode(src), os.fsencode(dst), 0)For precise equivalence with `cp -a`, you would use `CLONE_NOFOLLOW | CLONE_ACL` here.
raise OSError(err, os.strerror(err), src)You probably want `, None, dst` too (sets the `filename2` attribute). The `None` is `winerror`, but it’s safely ignored on non-Windows.
except OSError:
subprocess.check_call(['cp', '-a', '-c', src, dest])I disagree with this fallback.
`cp -ac` is so much worse than the direct `clonefile` call*, and probably doesn’t actually do anything different than the direct `clonefile` call is intended to**. All the fallback does is silently paper over problems calling `clonefile`.
\* why is `cp -c` so bad? does it clone each file individually, rather than cloning the entire directory hierarchy in a single call?
\** `cp -c` can fall back to a non-clone copy if the filesystem doesn’t support clones, but you shouldn’t care because you detect whether clones are supported before trying it. Also, my experience has been that `cp -c` still doesn’t work across filesystems.
args.repository = os.path.realpath(args.repository)I didn’t re-test, but this will probably work.
gclient = os.path.realpath(os.path.join(args.repository, '.gclient'))You probably don’t need any other `realpath` now that you’re doing the big one above. Look elsewhere, too.
if args.reflink:This name is btrfs-specific. The name would ideally be made more generic, something about copy-on-write. You can hang on to `--reflink`/`--no-reflink` as deprecated options for compatibility, though.
(If you want to take this up, save it for a follow-up change.)
Related details
- Erik Staab
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 7 comments![]( "Open in Gerrit")
This file seems to `'single-quote'` string literals. Stick with that for consistency.
Done
res = _libc.clonefile(os.fsencode(src), os.fsencode(dst), 0)For precise equivalence with `cp -a`, you would use `CLONE_NOFOLLOW | CLONE_ACL` here.
Done
You probably want `, None, dst` too (sets the `filename2` attribute). The `None` is `winerror`, but it’s safely ignored on non-Windows.
Done
except OSError:
subprocess.check_call(['cp', '-a', '-c', src, dest])I disagree with this fallback.
`cp -ac` is so much worse than the direct `clonefile` call*, and probably doesn’t actually do anything different than the direct `clonefile` call is intended to**. All the fallback does is silently paper over problems calling `clonefile`.
\* why is `cp -c` so bad? does it clone each file individually, rather than cloning the entire directory hierarchy in a single call?
\** `cp -c` can fall back to a non-clone copy if the filesystem doesn’t support clones, but you shouldn’t care because you detect whether clones are supported before trying it. Also, my experience has been that `cp -c` still doesn’t work across filesystems.
Done
gclient = os.path.realpath(os.path.join(args.repository, '.gclient'))You probably don’t need any other `realpath` now that you’re doing the big one above. Look elsewhere, too.
Done
This name is btrfs-specific. The name would ideally be made more generic, something about copy-on-write. You can hang on to `--reflink`/`--no-reflink` as deprecated options for compatibility, though.
(If you want to take this up, save it for a follow-up change.)
- Erik Staab
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 2 comments![]( "Open in Gerrit")
LGTM
def adopt_git_worktree(src, dest):This function only works in “reflink” mode now. You want to document that at least.
Related details
- Erik Staab
- Justin Cohen
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Justin Cohen (Gerrit)
Justin Cohen added 1 comment![]( "Open in Gerrit")
This function only works in “reflink” mode now. You want to document that at least.
- Erik Staab
- Mark Mentovai
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mark Mentovai (Gerrit)
Mark Mentovai added 1 comment![]( "Open in Gerrit")
LGTM
- Erik Staab
- Justin Cohen
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |