Bug #82847 for AnyEvent-Filesys-Notify: _process_events rescan is inefficient, too slow on large directories

Mon Jan 21 03:58:01 2013 cpan [...] wellrounded.com - Ticket created

Subject:

_process_events rescan is inefficient, too slow on large directories

Comments in _process_events() say: # We are just ingoring the raw events for now... Mac::FSEvents # doesn't provide much information, so rescan ourselves The rescan works poorly when the directories contain a large number of files. Every time an inotify event occurs, this routine computes a stat on EVERY file in the directories. Unfortunately, this makes AnyEvent::Filesys::Notify unusable in my application, running under Linux. It's just too slow. Instead, _process_events() SHOULD process the raw events, and directly translate each Linux::Inotify2 event directly into the corresponding AEFN::Event. No rescan is needed. If you are concerned about Mac OSX performance, can you break that case into a separate routine? That way, Linux users will not have to suffer performance degradation to accommodate Mac code. Thank you.

Wed Jan 23 09:27:40 2013 mgrimes [...] cpan.org - Correspondence added

Jonathan, I agree, parsing the events where available would likely be the most efficient implementation. In my experience though, this can be a bit tricky. There are nuances (such as multiple events fired for the same change) that need to dealt with. I will look into making this change as time permits. Of course, I would welcome a patch. Thanks, Mark

Wed Jan 23 09:27:41 2013 The RT System itself - Status changed from 'new' to 'open'

Tue Jun 18 15:35:17 2013 mgrimes [...] cpan.org - Ticket deleted

Tue Jun 18 15:35:19 2013 mgrimes [...] cpan.org - Status changed from 'deleted' to 'open'

Sun Dec 08 04:03:24 2013 carsten [...] wolffcarsten.de - Correspondence added

Subject:	Re: [rt.cpan.org #82847] Patch proposal
Date:	Sun, 08 Dec 2013 10:03:07 +0100
To:	bug-AnyEvent-Filesys-Notify [...] rt.cpan.org
From:	Carsten Wolff <carsten [...] wolffcarsten.de>

Hi, how about the attached patch? The comment describes two cases, where the behavior deviates from the Notify.pm code. What do you think about these cases? I feel the patch behavior is more correct, but are you OK with this? The performance advantage of this patch is big. Previously, when I touched 1000 files at once, my daemon would use 100% CPU for 6 minutes, which also means a delay of 6 minutes for the last event to be generated. With the patch, processing of the same test takes 1 or 2 seconds. Cheers Carsten

Message body is not shown because sender requested not to inline it.

Sun Dec 08 11:54:19 2013 cpan [...] wellrounded.com - Correspondence added

Thanks, Carsten. The patch looks OK to me. I would make the following suggestions. - Your change could break existing code that uses AEFN. In my opinion, it should be possible for someone to upgrade without breaking anything. If you're going to change the event behavior as described, make it backward-compatible. That is, add a flag to explicitly select the new behavior, and make it default to the old behavior. (That said, I have no strong opinion on which behavior is "more correct.") - Is it possible that $self->cb might be undef? If so, your code would crash when you try to apply it at the end of the sub. - Would it make sense to synthesize a single "moved" AEFN:Event from the two MOVED_FROM and MOVED_TO events? That would be really cool and useful. Best wishes, --Jonathan

Mon Dec 09 08:35:47 2013 carsten [...] wolffcarsten.de - Correspondence added

Subject:	Re: [rt.cpan.org #82847] Patch proposal
Date:	Mon, 09 Dec 2013 14:35:24 +0100
To:	bug-anyevent-filesys-notify [...] rt.cpan.org
From:	Carsten Wolff <carsten [...] wolffcarsten.de>

Hi Jonathan, Jonathan wrote: Show quoted text

> Thanks, Carsten. The patch looks OK to me.

thanks for the review. Show quoted text

> - Your change could break existing code that uses AEFN. In my opinion, it > should be possible for someone to upgrade without breaking anything. If > you're going to change the event behavior as described, make it > backward-compatible.

Sure, that makes sense. Find attached a v2 of the patch. Show quoted text

> - Is it possible that $self->cb might be undef? If so, your code would > crash when you try to apply it at the end of the sub.

No, the member var has "required => 1". Show quoted text

> - Would it make sense to synthesize a single "moved" AEFN:Event from the > two MOVED_FROM and MOVED_TO events? That would be really cool and useful.

I see two problems with this idea: 1. A single AEFN event has only one "path". How would you transport the information about old and new name together in one Event? 2. The Linux::Inotify2 watcher only passes one event at a time to the callback (_process_events). To associate and/or merge two events with each other, there would need to exist a local event-queue which would hold and delay the first event up to the arrival of the second. Cheers Carsten

Message body is not shown because sender requested not to inline it.

Mon Dec 09 12:38:28 2013 mgrimes [...] cpan.org - Correspondence added

Thanks for the patch Carsten, and thanks for the comments Jonathan. The patch looks good. I've applied/modified the patch, added some tests and refactored the testing method. The old testing method failed since we no longer receive all the events at once. The new method seems more robust and simpler. There were a couple of issues that I addressed. Most importantly, if we `mkdir subdir && touch subdir/file` the subdir/file created event is missed. This is because we need to catch the subdir created event, then add subdir to the list of watched directories. In the time that take, we miss the subdir/file created event from Inotify2. AEFN now walks new subdirs, generating 'created' events for children. I've pushed this to github as a new branch 'rv/efficient-events'. Let me know if you have any comments, otherwise I'll probably release to cpan in the next day or so. https://github.com/mvgrimes/AnyEvent-Filesys-Notify/tree/rv/efficient-events Thanks again, Mark

Tue Dec 10 01:46:50 2013 cpan [...] wellrounded.com - Correspondence added

Carsten, Thanks again for your work on this issue, and for your feedback. In response to your questions about the idea of combining the MOVED_FROM and MOVED_TO pair of events events into a single AEFN::Event, I would say: (1) The AEFN::Event data structure would have to be modified to carry both paths. (2) Yes, the first event would have to be stored, in a hash or an array, until the second event arrives. In the case of a Move event, the two events happen almost simultaneously. So there would be essentially no delay from combining them. Nevertheless, this would be a separate project, and is beyond the scope of the bugfix. It's just an idea that could potentially simplify the processing of Move notifications. I am not planning to submit an implementation. Best, --Jonathan On Mon Dec 09 08:35:47 2013, carsten@wolffcarsten.de wrote: Show quoted text

> Hi Jonathan, > > Jonathan wrote:

> > Thanks, Carsten. The patch looks OK to me.

> > thanks for the review. >

> > - Your change could break existing code that uses AEFN. In my > > opinion, it > > should be possible for someone to upgrade without breaking anything. > > If > > you're going to change the event behavior as described, make it > > backward-compatible.

> > Sure, that makes sense. Find attached a v2 of the patch. >

> > - Is it possible that $self->cb might be undef? If so, your code > > would > > crash when you try to apply it at the end of the sub.

> > No, the member var has "required => 1". >

> > - Would it make sense to synthesize a single "moved" AEFN:Event from > > the > > two MOVED_FROM and MOVED_TO events? That would be really cool and > > useful.

> > I see two problems with this idea: > 1. A single AEFN event has only one "path". How would you transport > the > information about old and new name together in one Event? > 2. The Linux::Inotify2 watcher only passes one event at a time to the > callback > (_process_events). To associate and/or merge two events with each > other, there > would need to exist a local event-queue which would hold and delay > the first > event up to the arrival of the second. > > Cheers > Carsten

Tue Dec 10 01:49:17 2013 cpan [...] wellrounded.com - Correspondence added

Mark, Thank you for pushing this patch forward. I think it will make users much happier with AEFN::Event. --Jonathan On Mon Dec 09 12:38:28 2013, MGRIMES wrote: Show quoted text

> Thanks for the patch Carsten, and thanks for the comments Jonathan. > > The patch looks good. I've applied/modified the patch, added some > tests and refactored the testing method. The old testing method failed > since we no longer receive all the events at once. The new method > seems more robust and simpler. > > There were a couple of issues that I addressed. Most importantly, if > we `mkdir subdir && touch subdir/file` the subdir/file created event > is missed. This is because we need to catch the subdir created event, > then add subdir to the list of watched directories. In the time that > take, we miss the subdir/file created event from Inotify2. AEFN now > walks new subdirs, generating 'created' events for children. > > I've pushed this to github as a new branch 'rv/efficient-events'. Let > me know if you have any comments, otherwise I'll probably release to > cpan in the next day or so. > > https://github.com/mvgrimes/AnyEvent-Filesys-Notify/tree/rv/efficient- > events > > Thanks again, > Mark

Tue Dec 10 05:12:50 2013 carsten [...] wolffcarsten.de - Correspondence added

Subject:	Re: [rt.cpan.org #82847] Patch proposal
Date:	Tue, 10 Dec 2013 11:12:33 +0100
To:	bug-anyevent-filesys-notify [...] rt.cpan.org
From:	Carsten Wolff <carsten [...] wolffcarsten.de>

Hi Mark, thanks for the quick turnaround! Show quoted text

> There were a couple of issues that I addressed. Most importantly, if we > `mkdir subdir && touch subdir/file` the subdir/file created event is > missed. This is because we need to catch the subdir created event, then add > subdir to the list of watched directories. In the time that take, we miss > the subdir/file created event from Inotify2. AEFN now walks new subdirs, > generating 'created' events for children.

That's a nice catch. Show quoted text

> I've pushed this to github as a new branch 'rv/efficient-events'. Let me > know if you have any comments, otherwise I'll probably release to cpan in > the next day or so. > > https://github.com/mvgrimes/AnyEvent-Filesys-Notify/tree/rv/efficient-events

For me, the branch looks good. Thanks for taking the time to polish this up! Cheers Carsten

Tue Dec 10 11:52:15 2013 mgrimes [...] cpan.org - Correspondence added

New version 1.10 released on CPAN. Thanks all.

Tue Dec 10 11:52:15 2013 mgrimes [...] cpan.org - Status changed from 'open' to 'resolved'

Bug #82847 for AnyEvent-Filesys-Notify: _process_events rescan is inefficient, too slow on large directories

Preferred bug tracker