Search $PATH for a binary rather than shelling out to `which` #1573

Merged
merged 1 commit into from Dec 3, 2011

Conversation

Projects
None yet
6 participants
@tenderlove
Contributor

tenderlove commented Dec 3, 2011

I'm trying to speed up startup time for rake environment (on rails). Creating subshells came up on my list, so I'm trying to eliminate them.

This patch changes Bundler to search the PATH environment variable rather than shelling out to which. The function isn't 100% perfect as it doesn't handle "./sudo" the same way the shell does, but our input for this function is known so I don't think it matters.

@sferik

This comment has been minimized.

Show comment Hide comment
@sferik

sferik Dec 3, 2011

Member

Looks good. Thanks for your continued work on Rails performance improvements.

Member

sferik commented Dec 3, 2011

Looks good. Thanks for your continued work on Rails performance improvements.

sferik added a commit that referenced this pull request Dec 3, 2011

Merge pull request #1573 from tenderlove/shell
Search $PATH for a binary rather than shelling out to `which`

@sferik sferik merged commit 36c9f92 into bundler:master Dec 3, 2011

@wycats

This comment has been minimized.

Show comment Hide comment
@wycats

wycats Dec 3, 2011

Contributor

@tenderlove - just curious... did you benchmark the difference (i.e. is the newer code actually faster than shelling out)?

Contributor

wycats commented Dec 3, 2011

@tenderlove - just curious... did you benchmark the difference (i.e. is the newer code actually faster than shelling out)?

@sferik

This comment has been minimized.

Show comment Hide comment
@sferik

sferik Dec 3, 2011

Member

@fxn addressed your nitpick in 26fa77a.

Member

sferik commented Dec 3, 2011

@fxn addressed your nitpick in 26fa77a.

@fxn

This comment has been minimized.

Show comment Hide comment
@fxn

fxn Dec 3, 2011

Contributor

@sferik that was fast :).

Contributor

fxn commented Dec 3, 2011

@sferik that was fast :).

@tenderlove

This comment has been minimized.

Show comment Hide comment
@tenderlove

tenderlove Dec 3, 2011

Contributor

@wycats

require 'rubygems'
require 'benchmark/ips'

def sand_which cmd
  `which #{cmd}`
end

def blair_which cmd
  if File.executable? cmd
    cmd
  else
    path = ENV['PATH'].split(File::PATH_SEPARATOR).find { |path|
      File.executable? File.join(path, cmd)
    }
    path && File.expand_path(cmd, path)
  end
end

Benchmark.ips do |x|
  x.report("sand") { sand_which 'sudo' }
  x.report("blair") { blair_which 'sudo' }
end
[aaron@higgins git]$ ruby lol.rb 
                sand      251.0 (±4.8%) i/s -       1265 in   5.051489s (cycle=23)
               blair     3222.0 (±8.2%) i/s -      16218 in   5.076616s (cycle=306)
[aaron@higgins git]$

It's over 10x faster. Probably because the one that shells out has to shell out.

Contributor

tenderlove commented Dec 3, 2011

@wycats

require 'rubygems'
require 'benchmark/ips'

def sand_which cmd
  `which #{cmd}`
end

def blair_which cmd
  if File.executable? cmd
    cmd
  else
    path = ENV['PATH'].split(File::PATH_SEPARATOR).find { |path|
      File.executable? File.join(path, cmd)
    }
    path && File.expand_path(cmd, path)
  end
end

Benchmark.ips do |x|
  x.report("sand") { sand_which 'sudo' }
  x.report("blair") { blair_which 'sudo' }
end
[aaron@higgins git]$ ruby lol.rb 
                sand      251.0 (±4.8%) i/s -       1265 in   5.051489s (cycle=23)
               blair     3222.0 (±8.2%) i/s -      16218 in   5.076616s (cycle=306)
[aaron@higgins git]$

It's over 10x faster. Probably because the one that shells out has to shell out.

@sleeper

This comment has been minimized.

Show comment Hide comment
@sleeper

sleeper Dec 10, 2011

That's a pitty: I proposed the same change on 2011-11-03 (carlhuda#1516)... Today, nobody even had a look at it :(
Happy to see it merged by another channel, but kind of puzzled with the way mine has been handled ... Definitively not a way to encourage people to participate.

sleeper commented Dec 10, 2011

That's a pitty: I proposed the same change on 2011-11-03 (carlhuda#1516)... Today, nobody even had a look at it :(
Happy to see it merged by another channel, but kind of puzzled with the way mine has been handled ... Definitively not a way to encourage people to participate.

@sleeper sleeper referenced this pull request Dec 10, 2011

Closed

which: no sudo in ... #1321

@bfolkens

This comment has been minimized.

Show comment Hide comment
@bfolkens

bfolkens Apr 6, 2012

ENV['PATH'] is sometimes nil and breaks in a production environment I have with Passenger... Can we change it to ENV['PATH'].to_s or something to avoid that?

ENV['PATH'] is sometimes nil and breaks in a production environment I have with Passenger... Can we change it to ENV['PATH'].to_s or something to avoid that?

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