Skip to content
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

MAJOR changes for Listen 3.x (no TCP, no Celluloid) #319

Closed
e2 opened this issue Jun 12, 2015 · 7 comments
Closed

MAJOR changes for Listen 3.x (no TCP, no Celluloid) #319

e2 opened this issue Jun 12, 2015 · 7 comments

Comments

@e2
Copy link
Contributor

e2 commented Jun 12, 2015

Listen's priorities have drastically changed since it was created. Time to deal with major problems.

Current the major issues are:

  1. Hard to maintain codebase
  2. Performance issues on OSX (related to 1.)

Proposal for Listen 3.x

Proof-of-concept PR here: #318

  1. Removed TCP, since that should be in 2 separate gems (e.g. listen-server and listen-client)
  2. Removed Celluloid, since it's unnecessary in Listen itself (Listen should be a library and not an "app")
  3. Adapter config instead of global config (silencer, one Record object per watched directory)

Feedback

Highly appreciated here - even being picky about minor stuff (since that can suggest other important refactorings).

/cc - @thibaudgg , @rymai

@e2 e2 mentioned this issue Jun 12, 2015
@rymai
Copy link
Member

rymai commented Jun 12, 2015

Thanks @e2, I will try & review the proposal next week!

@thibaudgg
Copy link
Member

Hey @e2, sounds like all great ideas, I'm completely for an easier codebase to maintain.
Moving TCP stuff that are definitely not used by everybody to external gems seems like the way to go.

👍

@e2
Copy link
Contributor Author

e2 commented Jun 14, 2015

Ok, I'm done - there are a lot of changes here, but mostly removing code and reimplementing in new classes.

It's funny, but Listener's initialize method pretty much gives a bird's eye view of the architecture (and the messy dependencies):

eq_config = Event::Queue::Config.new(@config.relative?)
queue = Event::Queue.new(eq_config) { @processor.wakeup_on_event }
silencer = Silencer.new
rules = @config.silencer_rules
@silencer_controller = Silencer::Controller.new(silencer, rules)
@backend = Backend.new(dirs, queue, silencer, @config)
optimizer_config = QueueOptimizer::Config.new(@backend, silencer)
pconfig = Event::Config.new(
self,
queue,
QueueOptimizer.new(optimizer_config),
@backend.min_delay_between_events,
&block)
@processor = Event::Loop.new(pconfig)

Anyway - I plan to merge this into master, and release this as a 3.x prerelease.

I haven't tested performance properly, but here is a comparison with current 2.10:

3.x proposal here (forced polling):
3.65s user 0.15s system 10% cpu 37.337 total

current released 2.10 (forced polling):
17.58s user 1.90s system 53% cpu 36.274 total

(5 times less CPU usage - YMMV)

@thibaudgg
Copy link
Member

Awesome!

@mechanicalduck
Copy link

As I never was able to get listen running on windows because of issues with Celluloid (#246 #245),
I really appreciate these changes.

@e2
Copy link
Contributor Author

e2 commented Jun 15, 2015

@mechanicalduck -

Remove TCP functionality - does this mean that Listen cannot be used over network anymore?

TL;DR - very good question. In short, TCP has pretty much nothing to do with Listen, and custom implementations/setups will always outperform whatever Listen can provide. If you can, go to #258 and explain your use case accurately (what os you're listening on, what os you're receiving on, what kind of changes you're making, and what you're using, e.g. Guard).

Details:

I intend to release separate gems for TCP support. I indent to do it natively, because there's no need for Celluloid when running the TCP server and listeners in their own app. This should help with Windows issues also and VM setups. I may try to create a minimal TCP-only version of Listen, but I need use cases to make sure I properly set up the buffering, latency, queue optimizing, etc.

TCP has little in common with Listen - and this just obscured the codebase too much.

Some scenarios can be handled with specific tools, e.g. netcat, pipes, ssh, etc.

Examples of issues/conflicts:

  • the bin/listen script was good pretty much just for TCP, so it doesn't belong in this gem
  • the bin/listen script was too primitive for good control (no polling option, no latency, output control, multiple dir handling was recent, etc.)
  • TCP was just too tricky to implement properly, because of how events were buffered, optimized (paths didn't exist)
  • TCP is actually use-case specific (depending on what you're watching, with polling or not, where you're sending, how the dirs are setup, etc.)
  • extra dependency on Celluloid:IO, which cause Listen to depend on Celluloid by default anyway (due to architecture)
  • TCP is just too different (in terms of buffering) - so basically Listen had 2 independent "architectures" under the hood
  • TCP option handling was hideous, tricky, and randomly breaking (e.g. one of your above issues)
  • the TCP test suite was a time sink to get working (due to architecture and huge delays), while in practice the results were poor
  • TCP behaved fundamentally differently depending on whether polling or not - which made contributions impossible at times (without breaking things)

@e2
Copy link
Contributor Author

e2 commented Jun 27, 2015

Closing, since 3.x is released.

Linking to issue about extracting TCP: #258

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants