-
Notifications
You must be signed in to change notification settings - Fork 247
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
Allow running inotify in another process #583
base: master
Are you sure you want to change the base?
Conversation
cdc3548
to
d04af32
Compare
Process.kill("KILL", @pipe.pid) if process_running?(@pipe.pid) | ||
@pipe.close | ||
end | ||
rescue IOError, Errno::EBADF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint/HandleExceptions: Do not suppress exceptions.
dir = command[1..-1].chomp("\0") # remove status (M/A/D) and terminator null byte | ||
@callback.call([dir]) | ||
end | ||
rescue Interrupt, IOError, Errno::EBADF |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint/HandleExceptions: Do not suppress exceptions.
@callback = block | ||
end | ||
|
||
def run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metrics/MethodLength: Method has too many lines. [11/10]
lib/listen/adapter/process_linux.rb
Outdated
def _run | ||
dirs_to_watch = @callbacks.keys.map(&:to_s) | ||
worker = Worker.new(dirs_to_watch, &method(:_process_changes)) | ||
@worker_thread = Thread.new("worker_thread") { worker.run } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
true | ||
end | ||
|
||
def _configure(dir, &callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/EmptyMethod: Put empty method definitions on a single line.
|
||
module Listen | ||
module Adapter | ||
class ProcessLinux < Base |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/Documentation: Missing top-level class documentation comment.
lib/listen/adapter/process_linux.rb
Outdated
@@ -0,0 +1,87 @@ | |||
# frozen_string_literal: true | |||
|
|||
require "listen" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
d04af32
to
5a0c2c7
Compare
lib/listen/adapter/process_linux.rb
Outdated
command = @pipe.gets("\0") | ||
next unless command | ||
# remove status (M/A/D) and terminator null byte | ||
dir = command[1..].chomp("\0") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lint/Syntax: unexpected token tRBRACK
5a0c2c7
to
c7a1def
Compare
We're using Listen through
ActiveSupport::EventedFileUpdateChecker
on a large application, and we're seeing a big discrepancy between running on Linux and macOS.The main reason I found is that
rb-fsevent
uses a different process to do the file watching with which it communicates through a pipe.On the other hand,
rb-inotify
does syscalls throughffi
directly. In our case, recursing over all the subdirectories in Ruby takes a long time, about 1.3 seconds. By comparison, the same call toListen::Listener#start
takes less than 2 milliseconds on macOS.I've read through the Performance and Tips and Techniques sections of the README, however our issue is that we just have a lot of reloadable code in a lot of directories. This is after reducing the watched paths to a minimum already.
In this PR, I've introduced a new adapter,
ProcessLinux
, which instead of listening directly, forks to a process with the directories as arguments, and listens to the pipe. I've added a way to use it by adding a backend selecting optionprefer_fork
.It kinda works, however it doesn't fit in the current architecture very well. For example, the adapter doesn't go through the transitions properly.
Listener#start
returns before events can actually be processed.Also it doesn't handle pausing currently.
More importantly, deletions don't work for some reason. The calling process receives deletion events but they don't turn into a call to the callback.
Also we're still spending time creating all the snapshots in the calling process, even though I'm not sure we need this. Is the snapshot feature only necessary for pausing/restarting?
Anyway I've created a small repro script to show the issue around the directory recursion.
Repro script
I've also added some allocations because we're also seeing a lot of GC time while doing the directory recursion.
Results
Here are profiles: the Linux adapter, the forking Linux adapter, and for an unfair comparison the Darwin adapter (unfair because running on the machine while the others run in a container in a VM).
These can be opened using https://vernier.prof/. Unfortunately I can't link there directly because CORS.
We can see that the inotify implementation is still faster to receive the first event, but the call to
listener.start
is very slow (900ms).To test this, for now I've just changed the acceptance test to prefer forking, this shows a few failures:
I think the forking implementation shouldn't be necessary, and we could fix
rb_inotify
to do all the directory recursion in a C extension that releases the GVL. That should be enough to be able to start a listener in a thread without blocking the rest of the Ruby code. However I don't know how to do that, so this is my other attempt, just using forking to work around the GVL contention issue.Can you take a look and evaluate whether the adapter selecting option to choose between different available adapters for a given platform would be acceptable?
Can you provide pointers as to how to fix the issue I'm seeing with deleted files? And let me know if having a way to disable snapshots would make sense too, or a way to improve the performance of that feature.
Thanks!