Closed jonathanhefner closed 4 years ago
@ioquatix If you have a free moment, would you mind giving this proposal a look over? :pray: There are some Rails PRs I would like to make which would depend on this issue's resolution.
Do you have time to have a chat and go through the issue with me tomorrow?
If you can make a specific PR, I can review it and provide feedback. Honestly, I read your issue description and I think oh no... that seems like a complicated design we are trying to fix.
@ioquatix
Honestly, I read your issue description and I think oh no... that seems like a complicated design we are trying to fix.
I'm not sure if you meant the original code seems complicated, or my proposals seem complicated? I implemented the first proposal (replacing @listeners
) in #477. Please let me know what you think! If that change seems okay, I'll open a PR for the second proposal (replacing ThreadPool
).
I guess the problem here is global state.
ObjectSpace
is not compatible with JRuby very easily.
I think the solution is just to get rid of the global state. Is it possible?
I don't know if I can think of a way to iterate over all Listener
instances while avoiding both global state and ObjectSpace
. The Listen.stop
API seems unfortunately global by nature.
It's worth pointing out that Listen already appears to be broken on JRuby. But if JRuby compatibility is a future goal, I could perhaps bring back the @listeners
array and fill it with WeakRef
s instead. That would at least prevent the references from being held indefinitely.
@headius if you have a moment do you think you can take a look and comment on how we can make this work better and/or support JRuby?
The WeakRef
suggestion is a good one. Sadly, Ruby core still has not added any way to scan for empty weakrefs, so you'll have to poll them to clean them up, but it's better than having a hard reference.
The specs also pass for me locally on Darwin. I suspect that the specs that fail on Travis are launching a subprocess, since locally they run much slower than previous specs. Perhaps there's a timeout in place that's not giving the JRuby subprocess enough time?
I've merged all the changes. Can you please check it?
Yes, thank you @ioquatix! I wrote a benchmark script to show the difference:
require "bundler/inline"
gemfile(true) do
source "https://rubygems.org"
gem "benchmark-memory"
gem "listen", path: "."
end
require "benchmark/memory"
require "listen"
Benchmark.memory do |x|
x.report("warmup") do
Listen.to("lib") { }.start
Listen.stop
end
x.report("Listener#stop") do
Listen.to("lib") { }.tap(&:start).stop
end
end
Running in the listen
repo directory, on Linux...
Before (at ba5059c9f3a8594eae7e0050687c1a6735046d85):
warmup 928.393k memsize ( 123.073k retained)
8.968k objects ( 1.198k retained)
50.000 strings ( 50.000 retained)
Listener#stop 174.592k memsize ( 21.354k retained)
2.136k objects ( 155.000 retained)
50.000 strings ( 50.000 retained)
or
warmup 1.042M memsize ( 123.073k retained)
10.298k objects ( 1.198k retained)
50.000 strings ( 50.000 retained)
Listener#stop 1.109M memsize ( 1.060M retained)
797.000 objects ( 100.000 retained)
50.000 strings ( 27.000 retained)
depending on factors I can't seem to identify.
After (at f72d44bf3905b67bc1d117a62704665d8b4087d4):
warmup 952.683k memsize ( 134.827k retained)
9.281k objects ( 1.290k retained)
50.000 strings ( 50.000 retained)
Listener#stop 163.835k memsize ( 80.000 retained)
1.997k objects ( 2.000 retained)
50.000 strings ( 0.000 retained)
I've noticed at least two memory "leaks" when using on
Listener#stop
(notListen.stop
).The first leak is the
@listeners
array in theListen
module. It is appended to when creating aListener
, but only cleared inListen.stop
(notListener#stop
). I suppose a workaround would be to useListener.new
directly instead ofListen.to
, but that doesn't seem to be the recommended usage (going by the README). Also note that this leak isn't merely ofListener
instances, but of potentially large object graphs that eachListener
instance's block holds a reference to.The second leak is
Listen::Internals::ThreadPool
. It is appended to inListen::Adapter::Base#start
andListen::Event::Loop#setup
, but only cleared inListen.stop
(notListener#stop
).My proposal to fix the first leak is to remove
@listeners
and replace its usage inListen.stop
withObjectSpace.each_object
. It will be slower than iterating an array, but it shouldn't be done often, so that shouldn't be an issue.My proposal to fix the second leak is to remove
Listen::Internals::ThreadPool
entirely, and store thread references directly in eachListen::Adapter::Base
andListen::Event::Loop
instance. ThenListen::Adapter::Base#stop
andListen::Event::Loop#teardown
can kill and clear an instance's own threads.I am completely new to the Listen code base, so please tell me if I am misunderstanding the situation, or if my ideas are laughably naive. :sweat_smile: However, if these proposals sound good, I am willing to submit a PR.