Skip to content
This repository has been archived by the owner on Jul 4, 2023. It is now read-only.

cleanup: optimize perfermance #37409

Closed
wants to merge 1 commit into from
Closed

cleanup: optimize perfermance #37409

wants to merge 1 commit into from

Conversation

xu-cheng
Copy link
Member

@xu-cheng xu-cheng commented Mar 5, 2015

Only remove .DS_Store files when -s is passed.

On my Mac with 156 formulae installed, it will take about 10 seconds to
remove all the .DS_Store files

@xu-cheng
Copy link
Member Author

xu-cheng commented Mar 5, 2015

I just realized that it's so slow on my Mac is because I have MacTeX installed. There're 143631 files/folders inside /usr/local/texlive.

@dplarson
Copy link
Contributor

dplarson commented Mar 5, 2015

I've also got MacTeX installed and have noticed the same issue (i.e. that brew cleanup is slowed down in the removing .DS_Store files step). But perhaps it would be better to add a cleanup flag to ignore the .DS_Store removal step (instead of a flag to include removing .DS_Store) since most users probably don't have this issue.

@tdsmith
Copy link
Contributor

tdsmith commented Mar 5, 2015

Heh, same. Maybe skip the texlive path in rm_DS_Store instead?

@xu-cheng
Copy link
Member Author

xu-cheng commented Mar 5, 2015

Heh, same. Maybe skip the texlive path in rm_DS_Store instead?

There could be variety non-homebrew paths inside HOMEBREW_PREFIX. So I think it's better to remove .DS_Store files only in selective paths.

@MikeMcQuaid
Copy link
Member

Yeh, let's whitelist directories here instead of requiring a switch.

@xu-cheng
Copy link
Member Author

xu-cheng commented Mar 5, 2015

Updated.

@MikeMcQuaid
Copy link
Member

👍

%w[Cellar Frameworks Library bin etc include lib opt sbin share var].each do |path|
path = HOMEBREW_PREFIX/path
next unless path.exist?
quiet_system "find", path.to_s, "-name", ".DS_Store", "-delete"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could give all paths to find to speed up the process, i.e. use only quiet_system call instead of one per directory.

Update: Something like:

paths = %w[Cellar Frameworks Library bin etc include lib opt sbin share var].map { |p| HOMEBREW_PREFIX/p }
paths.select!(&:exist?)

quiet_system "find", paths * " ", "-name", ".DS_Store", "-delete"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could give all paths to find to speed up the process

Why it will speed up? It's the same amount folders which will be scanned.

quiet_system "find", paths * " ", "-name", ".DS_Store", "-delete"

This wouldn't work. It should be:

args = paths
args << %w[-name .DS_Store -delete]
quiet_system "find", *args

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it will speed up? It's the same amount folders which will be scanned.

Yes, but it fires find only once. Each call to quiet_system does the following:

  1. fork the current process
  2. redirect stdout and stderr to /dev/null
  3. exec the command
  4. wait for the command to end

find is waaay faster than Ruby since it’s compiled from optimized C. I just tested the following on my machine:

# create 1000 directories with a lot of subdirectories
$ for i in {1..1000}; do d=dir$i/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r; mkdir -p $d; touch $d/{x,y,z}; done

# search for all "x" files with `find`
$ time find dir{1..1000} -name x >/dev/null

real    0m0.272s
user    0m0.057s
sys 0m0.215s

# do the same in Ruby
$ brew irb
>>> b=Time.now;(1..1000).each { |i| quiet_system "find", "dir#{i}", "-name", "x" };Time.now-b
5.933559

That’s just a test, but it took 20+ times more time to do the loop in Ruby than letting find do it itself.

This wouldn't work.

Right, sorry.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Only remove .DS_Store files from whitelisted directories.
@xu-cheng xu-cheng closed this in 86205ca Mar 7, 2015
@xu-cheng xu-cheng deleted the cleanup branch March 7, 2015 04:05
@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants