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

[path_list.rb] Improve performance of read_file_system #5927

Merged
merged 3 commits into from Sep 23, 2016

Conversation

Projects
None yet
4 participants
@hborders

hborders commented Sep 21, 2016

  • Used a simple String.slice to create relative paths instead of path.relative_path_from(root). Since we already know that all files must exist under root (because we globbed absolute_paths from root), we can just remove the first root.to_s.length + 1 characters from an absolute_path to get a relative_path.
  • Partition dirs and files first since we already get an array of absolute paths back from Pathname.glob, removing unnecessary string concat: File.directory?(root + path)
  • Used lazy arrays to reduce array iteration.

I profiled my code before the change.
I profiled my new code again, and read_file_system practically disappears.

previous:

real     0m59.095s
user     0m46.133s
sys      0m2.360s

now:

real     0m23.712s
user     0m14.492s
sys      0m2.030s
Heath Borders
[path_list.rb] Improve performance of read_file_system
* Used a simple `String.slice` to create relative paths instead of `path.relative_path_from(root)`. Since we already know that all files must exist under `root` (because we globbed `absolute_paths` from `root`), we can just remove the first `root.to_s.length + 1` characters from an `absolute_path` to get a `relative_path`.
* Partition `dirs` and `files` first since we already get an array of absolute paths back from `Pathname.glob`, removing unnecessary string concat: `File.directory?(root + path)`
* Used `lazy` arrays to reduce array iteration.

I [profiled my code before the change](https://github.com/CocoaPods/CocoaPods/files/477973/call-stack.zip).
I [profiled my new code again](https://github.com/CocoaPods/CocoaPods/files/478512/call-stack2.html.zip), and `read_file_system` practically disappears.

previous:

```
real     0m59.095s
user     0m46.133s
sys      0m2.360s
```

now:

```
real     0m23.712s
user     0m14.492s
sys      0m2.030s
```
@hborders

This comment has been minimized.

hborders commented Sep 21, 2016

This fixes #5890

Heath Borders
@segiddins

Other than the one nit, this looks pretty good to me

dirs, files = dirs_and_files.partition { |path| File.directory?(path) }
root_length = root.cleanpath.to_s.length + File::SEPARATOR.length
relative_sorted = lambda do |paths|

This comment has been minimized.

@segiddins

segiddins Sep 22, 2016

Member

sort_relative_paths?

This comment has been minimized.

@hborders

hborders Sep 22, 2016

I want to make it clear that the lambda accepts full paths, but returns relative paths. sort_relative_paths sounds like it accepts relative paths. Maybe sorted_relative_paths_from_full_paths ?

This comment has been minimized.

@segiddins
@hborders

This comment has been minimized.

hborders commented Sep 22, 2016

Are these failures normal? I don't know what to do to correct them.

@segiddins

This comment has been minimized.

Member

segiddins commented Sep 22, 2016

Unfortunately, they are :(

@hborders

This comment has been minimized.

hborders commented Sep 22, 2016

Do I need to do anything else before this can be merged?

(I'm not trying to pressure you to merge me sooner. I've never contributed to this project before, so I don't know what to expect.)

@dantoml

This comment has been minimized.

Member

dantoml commented Sep 22, 2016

@hborders hey, sorry have been super busy this week. This LGTM. I just restarted the system Ruby job on Travis, but will test manually tomorrow if it remains flaky.

@hborders

This comment has been minimized.

hborders commented Sep 22, 2016

No worries. I'm just super excited for my first contribution. I'll wait patiently from now on. :)

@dantoml

This comment has been minimized.

Member

dantoml commented Sep 22, 2016

@hborders Thanks for the contribution 💜 - I've wanted to spend some time digging into this for a while.

System Ruby job appears to have passed, running the others.

@dantoml

LGTM with CI

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 23, 2016

@hborders nicely done! This along a couple of other fixes we've done will make pod install super fast.

@dantoml

This comment has been minimized.

Member

dantoml commented Sep 23, 2016

The last matrix failure is transitive 👍👍

@dnkoutso

This comment has been minimized.

Contributor

dnkoutso commented Sep 23, 2016

1.1.0 is shaping up to be a great performance release.

@dantoml

This comment has been minimized.

Member

dantoml commented Sep 23, 2016

:shipit: - thanks @hborders

@dantoml dantoml merged commit fffa804 into CocoaPods:master Sep 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dantoml dantoml referenced this pull request Sep 23, 2016

Closed

pod install takes 48 seconds for me :( #5890

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment