Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize passing multiple paths to usort CLI #161

Merged
merged 1 commit into from
May 19, 2022

Conversation

amyreese
Copy link
Member

@amyreese amyreese commented May 17, 2022

Previously, when passing multiple paths to the usort CLI, it would
call usort_path() separately for each path given, preventing any
chance of parallel processing. This means usort check usort/**/*.py
would take ~10X longer than usort check usort, and also results in
double formatting if a path appeared or was a child of multiple
arguments:

(.venv) jreese@butterfree ~/workspace/usort main  » time usort check usort
usort check usort  3.64s user 0.32s system 338% cpu 1.169 total

(.venv) jreese@butterfree ~/workspace/usort main  » time usort check usort usort usort
usort check usort usort usort  10.67s user 0.89s system 470% cpu 2.458 total

(.venv) jreese@butterfree ~/workspace/usort main  » time usort check usort/**/*.py usort/**/*.py usort/**/*.py
usort check usort/**/*.py usort/**/*.py usort/**/*.py  13.41s user 1.59s system 62% cpu 24.141 total

With the changes from #160, this is partially masked by removing the
cost of starting child processes, but still results in linear time
increase when passing many arguments:

(.venv) jreese@butterfree ~/workspace/usort patch-1  » time usort check usort
usort check usort  3.68s user 0.36s system 318% cpu 1.267 total

(.venv) jreese@butterfree ~/workspace/usort patch-1  » time usort check usort usort usort
usort check usort usort usort  10.37s user 0.86s system 483% cpu 2.323 total

(.venv) jreese@butterfree ~/workspace/usort patch-1  » time usort check usort/**/*.py usort/**/*.py usort/**/*.py
usort check usort/**/*.py usort/**/*.py usort/**/*.py  2.45s user 0.06s system 89% cpu 2.803 total

This builds on #160 by updating usort_path() to optionally take an
iterable of Path objects, and materializes a set of paths to sort before
actually starting child processes. This allows maximum utilization of
multiprocessing, and also eliminates any double-formatting if the CLI
arguments include any overlap of paths:

(.venv) jreese@butterfree ~/workspace/usort gather  ‹1› » time usort check usort
usort check usort  3.82s user 0.36s system 333% cpu 1.255 total

(.venv) jreese@butterfree ~/workspace/usort gather  » time usort check usort usort usort
usort check usort usort usort  3.88s user 0.37s system 356% cpu 1.192 total

(.venv) jreese@butterfree ~/workspace/usort gather  » time usort check usort/**/*.py usort/**/*.py usort/**/*.py
usort check usort/**/*.py usort/**/*.py usort/**/*.py  3.68s user 0.36s system 323% cpu 1.249 total

@amyreese amyreese added this to the 1.1 milestone May 17, 2022
@amyreese amyreese requested a review from thatch May 17, 2022 23:46
@amyreese amyreese self-assigned this May 17, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 17, 2022
Previously, when passing multiple paths to the usort CLI, it would
call `usort_path()` separately for each path given, preventing any
chance of parallel processing. This means `usort check usort/**/*.py`
would take ~10X longer than `usort check usort`, and also results in
double formatting if a path appeared or was a child of multiple
arguments:

```
(.venv) jreese@butterfree ~/workspace/usort main  » time usort check usort
usort check usort  3.64s user 0.32s system 338% cpu 1.169 total

(.venv) jreese@butterfree ~/workspace/usort main  » time usort check usort usort usort
usort check usort usort usort  10.67s user 0.89s system 470% cpu 2.458 total

(.venv) jreese@butterfree ~/workspace/usort main  » time usort check usort/**/*.py usort/**/*.py usort/**/*.py
usort check usort/**/*.py usort/**/*.py usort/**/*.py  13.41s user 1.59s system 62% cpu 24.141 total
```

With the changes from facebook#160, this is partially masked by removing the
cost of starting child processes, but still results in linear time
increase when passing many arguments:

```
(.venv) jreese@butterfree ~/workspace/usort patch-1  » time usort check usort
usort check usort  3.68s user 0.36s system 318% cpu 1.267 total

(.venv) jreese@butterfree ~/workspace/usort patch-1  » time usort check usort usort usort
usort check usort usort usort  10.37s user 0.86s system 483% cpu 2.323 total

(.venv) jreese@butterfree ~/workspace/usort patch-1  » time usort check usort/**/*.py usort/**/*.py usort/**/*.py
usort check usort/**/*.py usort/**/*.py usort/**/*.py  2.45s user 0.06s system 89% cpu 2.803 total
```

This builds on facebook#160 by updating `usort_path()` to optionally take an
iterable of Path objects, and materializes a set of paths to sort before
actually starting child processes. This allows maximum utilization of
multiprocessing, and also eliminates any double-formatting if the CLI
arguments include any overlap of paths:

```
(.venv) jreese@butterfree ~/workspace/usort gather  ‹1› » time usort check usort
usort check usort  3.82s user 0.36s system 333% cpu 1.255 total

(.venv) jreese@butterfree ~/workspace/usort gather  » time usort check usort usort usort
usort check usort usort usort  3.88s user 0.37s system 356% cpu 1.192 total

(.venv) jreese@butterfree ~/workspace/usort gather  » time usort check usort/**/*.py usort/**/*.py usort/**/*.py
usort check usort/**/*.py usort/**/*.py usort/**/*.py  3.68s user 0.36s system 323% cpu 1.249 total
```
@amyreese amyreese merged commit aed4453 into facebook:main May 19, 2022
@amyreese amyreese deleted the gather branch May 19, 2022 16:56
amyreese added a commit to amyreese/usort that referenced this pull request Jul 22, 2022
Optimize passing multiple paths to usort CLI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants