Accumulate changes #203

alexch · 2014-03-08T13:16:34Z

See commit comment alexch@3ecc5e3 for details.

I had to fiddle with the timing in some tests because the listener now waits a bit before calling back (just in case there are more changes coming).

My unit test is pretty gnarly, and just tests that two changes arriving one after another both get received... it doesn't really test that they get accumulated together into one callback. Think I should expand it, or just delete it and rely on acceptance tests? (which also currently don't test that the changes are accumulated)

…cumulate before calling block. Fixes guard#202 The new algorithm is (pseudocode): loop begin sleep options[:wait_for_delay] # wait for changes to accumulate new_changes = _pop_changes changes += new_changes end until new_changes.empty? unless changes.empty? block.call(changes) end end Note that the "sleep" happens both before and after any callback, so the value of wait_for_delay is effectively doubled. This also means that if the filesystem is constantly changing, the callback will never happen... maybe we should check for this and bail out after, say, 10 seconds even if new changes keep on coming.

alexch · 2014-03-08T13:34:56Z

OK, I made the test actually test the code :-)

Fixes #202

coveralls · 2014-03-08T13:51:23Z

Coverage increased (+0.08%) when pulling 3bc9ca3 on alexch:accumulate_changes into a86784b on guard:master.

thibaudgg · 2014-03-10T07:32:00Z

lib/listen/listener.rb

-          block.call(changes[:modified], changes[:added], changes[:removed])
+        changes = []
+        begin
+          sleep options[:wait_for_delay] # wait for changes to accumulate


You said:

However, it doesn't seem to be working quite as advertised. Instead of delaying when changes exist, it delays always, even if the changes don't exist. It's thus acting as an additional latency and slowing down responsiveness in all cases.

and I think is it still the case here, no?

On Mon, Mar 10, 2014 at 3:32 AM, Thibaud Guillaume-Gentil <
[email protected]> wrote:

You said:

However, it doesn't seem to be working quite as advertised. Instead of
delaying when changes exist, it delays always, even if the changes don't
exist. It's thus acting as an additional latency and slowing down
responsiveness in all cases.

and I think is it still the case here, no?

Not exactly. It always waits X seconds between checks. The old algorithm
would wait X + Y sec between checks if there were no changes, X sec if
there were changes.

Alex Chaffee - [email protected]
http://alexchaffee.com
http://twitter.com/alexch

Plus the main change is that it buffers up changes that happen in a
row but not "instantly".

Could you try with my proposal

Your proposal only re-pops once; mine keeps on going until there's
been no changes for X seconds.

Ok, but it that case the :wait_for_delay is not really a good arguments name, right?
Re-poping only once after the delay looks fine with :wait_for_delay name, if it's not enough you should just increase the delay no?

I'm not a big fan of the _smoosh_changes method, looks to complex for me. Updating _pop_changes() with _pop_changes(changes = { modified: [], added: [], removed: [] }) should be good no?

Ok, but it that case the :wait_for_delay is not really a good arguments name, right?
Re-poping only once after the delay looks fine with :wait_for_delay name, if it's not enough you should just increase the delay no?

So you're saying we should do another nested loop?

until any changes, wait X then until no changes, wait Y

I'm not a big fan of the _smoosh_changes method, looks to complex for me.

It's complex because the data structure is pretty weird -- we keep going back and forth between arrays and hashes and arrays-of-hashes. I thought I'd convert from array to hash right at the end before calling the callback, which allows us to say changes.empty? instead of changes.all? { |_,v| v.empty? } (which I for one found difficult to understand at first).

I'd be ok with changing it back but, silly name aside, I think the "smoosh" method makes things clearer.

Updating _pop_changes() with _pop_changes(changes = { modified: [], added: [], removed: [] }) should be good no?

Yeah, this too -- I personally find _pop_changes() much clearer than _pop_changes(changes = { modified: [], added: [], removed: [] }) :-)

I'm not saying that we should use another nested loop (oh no!), but I don't thing that sleeping again after each new change is a good idea (it could really delay a lot the call back if change doesn't stop to come).

For me just waiting once more and popping change back again should be fine enough (see my code example #203 (comment)).

Your approach seems also too complicated to me and add too many new logic IMHO (new_changes, _smoosh_changes)

I'm not horrified by nested loops; I was just clarifying what you meant.

Another clarification: the algorithm is separate from the implementation. I
can put back your weird hash-of-hashes code (I for one welcome our Open
Source Overlord) and will stop bickering (except to mention that the
"smoosh" complexity was already there, just in separate lines, and I pulled
it into a new, discrete, testable method)...

...but as for how long to wait, I'm honestly not sure which is better. The
use case I'm thinking of is a git pull or asset precompile or other case
where there's a clump of files arriving sequentially and then stopping to
arrive. In that case I'd like to keep waiting as long as necessary, until
all those files stop coming. Ideally (for both Guard and Rerun) there would
be only one restart, but in
#203 (comment) if we just
delay once, then maybe we're only halfway through, and there'd be another
clump of files coming right after, and we'd have to restart almost
immediately.

I get that we don't want to wait forever -- although in that case, which
is better, a server that never restarts or a server that restarts every
second? If the latter, what's a good value for the overall timeout? 2
seconds? 10 seconds?

I think the we want an option value that means "wait at least this long
between restarts" and we may need another value for "wait at most this long
between restarts"... and another for "wait this long before checking again"
(which is 0.1 now & I agree that one should be de minimis).

This is fun! Or is it irritating? Hard to tell.

Ok I understand better your use case now, we can go like that then. Thanks for you patience :)

thibaudgg · 2014-03-10T07:34:43Z

Could you try with my proposal, it looks simpler to me:

       loop do
          break if @stopping

          changes = _pop_changes
          if changes.all? { |_,v| v.empty? }
            sleep 0.1
          else
            sleep options[:wait_for_delay]
            changes = _pop_changes(changes) # pop again after the delay
            block.call(changes[:modified], changes[:added], changes[:removed])
         end
       end

# update _pop_changes 
def _pop_changes(changes = { modified: [], added: [], removed: [] })

thibaudgg · 2014-03-10T07:35:45Z

spec/support/acceptance_helper.rb

@@ -1,9 +1,9 @@
-def listen
-  sleep 0.5 # wait for changes
+def listen lag = 0.5


Should be def listen(lag = 0.5) :) https://github.com/bbatsov/ruby-style-guide

coveralls · 2014-03-11T14:32:09Z

Coverage remained the same when pulling 46d390a on alexch:accumulate_changes into a86784b on guard:master.

thibaudgg · 2014-03-13T14:28:32Z

lib/listen/listener.rb

+          changes += new_changes
+        end until new_changes.empty?
+        unless changes.empty?
+          hash = _smoosh_changes changes


Should be hash = _smoosh_changes(changes)

fixed in amended commit 42f4258

Improve how `wait_for_delay` option is handled

thibaudgg · 2014-03-14T07:24:22Z

Thanks, 2.7.1 released!

alexch added 3 commits March 8, 2014 04:07

ignore .idea (RubyMine project dir)

3ec4a2d

improve wait_for_changes test

ac96869

fiddle with test timing

3bc9ca3

thibaudgg reviewed Mar 10, 2014
View reviewed changes

thibaudgg reviewed Mar 13, 2014
View reviewed changes

comply with https://github.com/bbatsov/ruby-style-guide

42f4258

thibaudgg added a commit that referenced this pull request Mar 14, 2014

Merge pull request #203 from alexch/accumulate_changes

c538bb1

Improve how `wait_for_delay` option is handled

thibaudgg merged commit c538bb1 into guard:master Mar 14, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accumulate changes #203

Accumulate changes #203

alexch commented Mar 8, 2014

alexch commented Mar 8, 2014

coveralls commented Mar 8, 2014

thibaudgg Mar 10, 2014

alexch Mar 10, 2014

alexch Mar 10, 2014

alexch Mar 10, 2014

thibaudgg Mar 10, 2014

alexch Mar 11, 2014

thibaudgg Mar 12, 2014

alexch Mar 12, 2014

thibaudgg Mar 13, 2014

thibaudgg commented Mar 10, 2014

thibaudgg Mar 10, 2014

coveralls commented Mar 11, 2014

thibaudgg Mar 13, 2014

alexch Mar 13, 2014

thibaudgg commented Mar 14, 2014

Accumulate changes #203

Accumulate changes #203

Conversation

alexch commented Mar 8, 2014

alexch commented Mar 8, 2014

coveralls commented Mar 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaudgg commented Mar 10, 2014

Choose a reason for hiding this comment

coveralls commented Mar 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaudgg commented Mar 14, 2014