Improve crawling performance of `Notifier#watch`

Summary

I'd like to propose a few changes to Notifier#watch method to make it more performant. rb-inotify is an essential part of Rails' evented file watcher setup and since at least at this moment it's highly coupled with application boot process - lowering the time it takes to setup watcher and crawl directories will directly impact Rails app developer experience.

Proposed changes

There are several changes I'm proposing, some of those are the most impactful, some are cosmetic and I haven't benchmarked impact of each change, I just benchmarked it as a whole:

Use Dir.each_child which automatically skips . and .. entries which allows us to avoid doing regexp matching and building binary_d path. This is most likely the most significant change out of all.
Move File.directory?(d) check higher since we will be iterating over many files and it's better to return earlier
Since RECURSIVE_BLACKLIST has only one entry - turn it into a String constant and compare the exact value instead of using includes?
We don't need to call flags.include?(:dont_follow) on each iteration as nothing modifies flags - the check can be moved out of the loop
Check next if NON_RECURSIVE == d last as it's the least likely condition to cause an iteration skip so there is no need to check it early.

Benchmark

DIRECTORIES_LIST could have just one entry pointing to the root of a Rails app, but it's an array because Rails will only watch autoloaded directories and this is what I was initially benchmarking against.

New vs Old benchmark

```ruby RECURSIVE_BLACKLIST = ["/dev/fd"] NON_ENUMERABLE = "/dev/fd" DIRECTORIES_LIST = [] # you could put just one entry which is the root directory of a Rails app, for example def watch(path, *flags, &callback) return unless flags.include?(:recursive) dir = Dir.new(path) dir.each do |base| d = File.join(path, base) binary_d = d.respond_to?(:force_encoding) ? d.dup.force_encoding("BINARY") : d next if %r{/\.\.?$}.match?(binary_d) # Current or parent directory next if RECURSIVE_BLACKLIST.include?(d) next if flags.include?(:dont_follow) && File.symlink?(d) next unless File.directory?(d) watch(d, *flags, &callback) end dir.close rec_flags = [:create, :moved_to] watch(path, *((flags - [:recursive]) | rec_flags)) end def new_watch(path, *flags, &callback) return unless flags.include?(:recursive) dont_follow = flags.include?(:dont_follow) Dir.each_child(path) do |base| d = File.join(path, base) next unless File.directory?(d) next if dont_follow && File.symlink?(d) next if d == NON_ENUMERABLE new_watch(d, *flags, &callback) end rec_flags = [:create, :moved_to] new_watch(path, *((flags - [:recursive]) | rec_flags)) end FLAGS = [:dont_follow, :recursive, :attrib, :create, :modify, :delete, :move, :close_write].freeze DIRECTORIES_LIST.each do |dir| watch(dir, *FLAGS) new_watch(dir, *FLAGS) end require "benchmark/ips" Benchmark.ips do |x| x.config(time: 30, warmup: 5) x.report("old") do DIRECTORIES_LIST.each do |dir| watch(dir, *FLAGS) end end x.report("new") do DIRECTORIES_LIST.each do |dir| new_watch(dir, *FLAGS) end end x.compare! end ```

Results:

Calculating -------------------------------------
                 old      0.403 (± 0.0%) i/s -     13.000 in  32.220443s
                 new      0.578 (± 0.0%) i/s -     18.000 in  31.140962s

Comparison:
                 new:        0.6 i/s
                 old:        0.4 i/s - 1.43x  slower

The old implementation seems to be ~1.4x slower (please have a good look at the benchmark to ensure two implementations are equivalent, I'm going to add an assertion that we iterate over the same list a bit later) but what's more important is how it impacts a real world Rails app boot time

Rails app boot time impact

There is no reason to look at the total boot time of a Rails app, but it should be sufficient to have a high-level perspective of what it takes to initialize a file watcher:

ms = Benchmark.ms { Rails.application.config.file_watcher.new(*Rails.application.watchable_args) { puts "this is a callback" } }

Results fluctuate a bit but before the proposed change our app was spending about ~850ms initializing one of the main file watchers and after the change it's closer to 500-550ms which is a reasonable outcome for us.

Tests

There should be no need for tests as it's purely performance-related changes.

Breaking changes

Is RECURSIVE_BLACKLIST considered to be a public constant? Is it okay to remove it?

guard / rb-inotify