[PATCH] inboxidle: avoid per-inbox anonymous subs

[PATCH] inboxidle: avoid per-inbox anonymous subs

[Eric Wong]

Anonymous subs cost over 5K each on x86-64.  So prefer the
less-recommended-but-still-documented way of using
Linux::Inotify2::watch to register watchers.

This also updates FakeInotify to detect modifications correctly
when used on systems with neither IO::KQueue nor
Linux::Inotify2.
---
 lib/PublicInbox/DirIdle.pm     | 13 +++++--
 lib/PublicInbox/FakeInotify.pm | 70 +++++++++++++++++-----------------
 lib/PublicInbox/InboxIdle.pm   | 19 +++++++--
 lib/PublicInbox/KQNotify.pm    | 36 ++++++++++-------
 t/fake_inotify.t               | 18 ++++-----
 t/kqnotify.t                   | 12 ++----
 6 files changed, 92 insertions(+), 76 deletions(-)

diff --git a/lib/PublicInbox/DirIdle.pm b/lib/PublicInbox/DirIdle.pm
index fbbc9531a20..2915fd6e2da 100644
--- a/lib/PublicInbox/DirIdle.pm
+++ b/lib/PublicInbox/DirIdle.pm
@@ -23,7 +23,7 @@ if ($^O eq 'linux' && eval { require Linux::Inotify2; 1 }) {
 
 sub new {
 	my ($class, $dirs, $cb) = @_;
-	my $self = bless {}, $class;
+	my $self = bless { cb => $cb }, $class;
 	my $inot;
 	if ($ino_cls) {
 		$inot = $ino_cls->new or die "E: $ino_cls->new: $!";
@@ -35,15 +35,20 @@ sub new {
 	}
 
 	# Linux::Inotify2->watch or similar
-	$inot->watch($_, $MAIL_IN, $cb) for @$dirs;
+	$inot->watch($_, $MAIL_IN) for @$dirs;
 	$self->{inot} = $inot;
+	PublicInbox::FakeInotify::poll_once($self) if !$ino_cls;
 	$self;
 }
 
 sub event_step {
 	my ($self) = @_;
-	eval { $self->{inot}->poll }; # Linux::Inotify2::poll
-	warn "$self->{inot}->poll err: $@\n" if $@;
+	my $cb = $self->{cb};
+	eval {
+		my @events = $self->{inot}->read; # Linux::Inotify2::poll
+		$cb->($_) for @events;
+	};
+	warn "$self->{inot}->read err: $@\n" if $@;
 }
 
 1;
diff --git a/lib/PublicInbox/FakeInotify.pm b/lib/PublicInbox/FakeInotify.pm
index debd2d39ae5..9275861368a 100644
--- a/lib/PublicInbox/FakeInotify.pm
+++ b/lib/PublicInbox/FakeInotify.pm
@@ -7,71 +7,66 @@ package PublicInbox::FakeInotify;
 use strict;
 use Time::HiRes qw(stat);
 use PublicInbox::DS;
-my $IN_CLOSE = 0x08 | 0x10; # match Linux inotify
+sub IN_MODIFY () { 0x02 } # match Linux inotify
 # my $IN_MOVED_TO = 0x80;
 # my $IN_CREATE = 0x100;
 sub MOVED_TO_OR_CREATE () { 0x80 | 0x100 }
 
 my $poll_intvl = 2; # same as Filesys::Notify::Simple
 
-sub poll_once {
-	my ($self) = @_;
-	eval { $self->poll };
-	warn "E: FakeInotify->poll: $@\n" if $@;
-	PublicInbox::DS::add_timer($poll_intvl, \&poll_once, $self);
-}
-
-sub new {
-	my $self = bless { watch => {} }, __PACKAGE__;
-	PublicInbox::DS::add_timer($poll_intvl, \&poll_once, $self);
-	$self;
-}
+sub new { bless { watch => {} }, __PACKAGE__ }
 
 # behaves like Linux::Inotify2->watch
 sub watch {
-	my ($self, $path, $mask, $cb) = @_;
+	my ($self, $path, $mask) = @_;
 	my @st = stat($path) or return;
 	my $k = "$path\0$mask";
-	$self->{watch}->{$k} = [ $st[10], $cb ]; # 10 - ctime
+	$self->{watch}->{$k} = $st[10]; # 10 - ctime
 	bless [ $self->{watch}, $k ], 'PublicInbox::FakeInotify::Watch';
 }
 
 sub on_new_files ($$$$) {
-	my ($dh, $cb, $path, $old_ctime) = @_;
+	my ($events, $dh, $path, $old_ctime) = @_;
 	while (defined(my $base = readdir($dh))) {
 		next if $base =~ /\A\.\.?\z/;
 		my $full = "$path/$base";
 		my @st = stat($full);
 		if (@st && $st[10] > $old_ctime) {
-			bless \$full, 'PublicInbox::FakeInotify::Event';
-			eval { $cb->(\$full) };
+			push @$events,
+				bless(\$full, 'PublicInbox::FakeInotify::Event')
 		}
 	}
 }
 
-# behaves like non-blocking Linux::Inotify2->poll
-sub poll {
+# behaves like non-blocking Linux::Inotify2->read
+sub read {
 	my ($self) = @_;
-	my $watch = $self->{watch} or return;
+	my $watch = $self->{watch} or return ();
+	my $events = [];
 	for my $x (keys %$watch) {
 		my ($path, $mask) = split(/\0/, $x, 2);
 		my @now = stat($path) or next;
-		my $prv = $watch->{$x};
-		my $cb = $prv->[-1];
-		my $old_ctime = $prv->[0];
-		if ($old_ctime != $now[10]) {
-			if (($mask & $IN_CLOSE) == $IN_CLOSE) {
-				eval { $cb->() };
-			} elsif ($mask & MOVED_TO_OR_CREATE) {
-				opendir(my $dh, $path) or do {
-					warn "W: opendir $path: $!\n";
-					next;
-				};
-				on_new_files($dh, $cb, $path, $old_ctime);
-			}
+		my $old_ctime = $watch->{$x};
+		$watch->{$x} = $now[10];
+		next if $old_ctime == $now[10];
+		if ($mask & IN_MODIFY) {
+			push @$events,
+				bless(\$path, 'PublicInbox::FakeInotify::Event')
+		} elsif ($mask & MOVED_TO_OR_CREATE) {
+			opendir(my $dh, $path) or do {
+				warn "W: opendir $path: $!\n";
+				next;
+			};
+			on_new_files($events, $dh, $path, $old_ctime);
 		}
-		@$prv = ($now[10], $cb);
 	}
+	@$events;
+}
+
+sub poll_once {
+	my ($obj) = @_;
+	$obj->event_step; # PublicInbox::InboxIdle::event_step
+	PublicInbox::DS::add_timer($poll_intvl, \&poll_once, $obj);
 }
 
 package PublicInbox::FakeInotify::Watch;
@@ -82,6 +77,11 @@ sub cancel {
 	delete $self->[0]->{$self->[1]};
 }
 
+sub name {
+	my ($self) = @_;
+	(split(/\0/, $self->[1], 2))[0];
+}
+
 package PublicInbox::FakeInotify::Event;
 use strict;
 
diff --git a/lib/PublicInbox/InboxIdle.pm b/lib/PublicInbox/InboxIdle.pm
index 59cb833fd5a..bdb30284ac0 100644
--- a/lib/PublicInbox/InboxIdle.pm
+++ b/lib/PublicInbox/InboxIdle.pm
@@ -36,12 +36,13 @@ sub in2_arm ($$) { # PublicInbox::Config::each_inbox callback
 		$ibx->{unlock_subs} and
 			die "BUG: $dir->{unlock_subs} should not exist";
 		$ibx->{unlock_subs} = $old_ibx->{unlock_subs};
-		$cur->[1]->cancel;
+		$cur->[1]->cancel; # Linux::Inotify2::Watch::cancel
 	}
 	$cur->[0] = $ibx;
 
 	my $lock = "$dir/".($ibx->version >= 2 ? 'inbox.lock' : 'ssoma.lock');
-	$cur->[1] = $inot->watch($lock, $IN_MODIFY, sub { $ibx->on_unlock });
+	my $w = $cur->[1] = $inot->watch($lock, $IN_MODIFY);
+	$self->{on_unlock}->{$w->name} = $ibx;
 
 	# TODO: detect deleted packs (and possibly other files)
 }
@@ -65,14 +66,24 @@ sub new {
 	}
 	$self->{inot} = $inot;
 	$self->{pathmap} = {}; # inboxdir => [ ibx, watch1, watch2, watch3...]
+	$self->{on_unlock} = {}; # lock path => ibx
 	refresh($self, $pi_config);
+	PublicInbox::FakeInotify::poll_once($self) if !$ino_cls;
 	$self;
 }
 
 sub event_step {
 	my ($self) = @_;
-	eval { $self->{inot}->poll }; # Linux::Inotify2::poll
-	warn "$self->{inot}->poll err: $@\n" if $@;
+	eval {
+		my @events = $self->{inot}->read; # Linux::Inotify2::read
+		my $on_unlock = $self->{on_unlock};
+		for my $ev (@events) {
+			if (my $ibx = $on_unlock->{$ev->fullname}) {
+				$ibx->on_unlock;
+			}
+		}
+	};
+	warn "{inot}->read err: $@\n" if $@;
 }
 
 # for graceful shutdown in PublicInbox::Daemon,
diff --git a/lib/PublicInbox/KQNotify.pm b/lib/PublicInbox/KQNotify.pm
index 9673b44290a..2305daa785b 100644
--- a/lib/PublicInbox/KQNotify.pm
+++ b/lib/PublicInbox/KQNotify.pm
@@ -19,16 +19,17 @@ sub new {
 }
 
 sub watch {
-	my ($self, $path, $mask, $cb) = @_;
-	my ($fh, $cls, @extra);
+	my ($self, $path, $mask) = @_;
+	my ($fh, $watch);
 	if (-d $path) {
 		opendir($fh, $path) or return;
 		my @st = stat($fh);
-		@extra = ($path, $st[10]); # 10: ctime
-		$cls = 'PublicInbox::KQNotify::Watchdir';
+		$watch = bless [ $fh, $path, $st[10] ],
+			'PublicInbox::KQNotify::Watchdir';
 	} else {
 		open($fh, '<', $path) or return;
-		$cls = 'PublicInbox::KQNotify::Watch';
+		$watch = bless [ $fh, $path ],
+			'PublicInbox::KQNotify::Watch';
 	}
 	my $ident = fileno($fh);
 	$self->{dskq}->{kq}->EV_SET($ident, # ident
@@ -37,11 +38,11 @@ sub watch {
 		$mask, # fflags
 		0, 0); # data, udata
 	if ($mask == NOTE_WRITE || $mask == MOVED_TO_OR_CREATE) {
-		$self->{watch}->{$ident} = [ $fh, $cb, @extra ];
+		$self->{watch}->{$ident} = $watch;
 	} else {
 		die "TODO Not implemented: $mask";
 	}
-	bless \$fh, $cls;
+	$watch;
 }
 
 # emulate Linux::Inotify::fileno
@@ -56,33 +57,40 @@ sub on_overflow {}
 sub blocking {}
 
 # behave like Linux::Inotify2::poll
-sub poll {
+sub read {
 	my ($self) = @_;
 	my @kevents = $self->{dskq}->{kq}->kevent(0);
+	my $events = [];
 	for my $kev (@kevents) {
 		my $ident = $kev->[KQ_IDENT];
 		my $mask = $kev->[KQ_FFLAGS];
-		my ($dh, $cb, $path, $old_ctime) = @{$self->{watch}->{$ident}};
-		if (!defined($path) && ($mask & NOTE_WRITE) == NOTE_WRITE) {
-			eval { $cb->() };
+		my ($dh, $path, $old_ctime) = @{$self->{watch}->{$ident}};
+		if (!defined($old_ctime)) {
+			push @$events,
+				bless(\$path, 'PublicInbox::FakeInotify::Event')
 		} elsif ($mask & MOVED_TO_OR_CREATE) {
 			my @new_st = stat($path) or next;
 			$self->{watch}->{$ident}->[3] = $new_st[10]; # ctime
 			rewinddir($dh);
-			PublicInbox::FakeInotify::on_new_files($dh, $cb,
+			PublicInbox::FakeInotify::on_new_files($events, $dh,
 							$path, $old_ctime);
 		}
 	}
+	@$events;
 }
 
 package PublicInbox::KQNotify::Watch;
 use strict;
 
-sub cancel { close ${$_[0]} or die "close: $!" }
+sub name { $_[0]->[1] }
+
+sub cancel { close $_[0]->[0] or die "close: $!" }
 
 package PublicInbox::KQNotify::Watchdir;
 use strict;
 
-sub cancel { closedir ${$_[0]} or die "closedir: $!" }
+sub name { $_[0]->[1] }
+
+sub cancel { closedir $_[0]->[0] or die "closedir: $!" }
 
 1;
diff --git a/t/fake_inotify.t b/t/fake_inotify.t
index f0db0cb58ec..11dac117f0d 100644
--- a/t/fake_inotify.t
+++ b/t/fake_inotify.t
@@ -16,29 +16,25 @@ close $fh or BAIL_OUT "close: $!";
 
 my $fi = PublicInbox::FakeInotify->new;
 my $mask = PublicInbox::FakeInotify::MOVED_TO_OR_CREATE();
-my $hit = [];
-my $cb = sub { push @$hit, map { $_->fullname } @_ };
-my $w = $fi->watch("$tmpdir/new", $mask, $cb);
+my $w = $fi->watch("$tmpdir/new", $mask);
 
 select undef, undef, undef, $MIN_FS_TICK;
 rename("$tmpdir/tst", "$tmpdir/new/tst") or BAIL_OUT "rename: $!";
-$fi->poll;
-is_deeply($hit, ["$tmpdir/new/tst"], 'rename(2) detected');
+my @events = map { $_->fullname } $fi->read;
+is_deeply(\@events, ["$tmpdir/new/tst"], 'rename(2) detected');
 
-@$hit = ();
 select undef, undef, undef, $MIN_FS_TICK;
 open $fh, '>', "$tmpdir/tst" or BAIL_OUT "open: $!";
 close $fh or BAIL_OUT "close: $!";
 link("$tmpdir/tst", "$tmpdir/new/link") or BAIL_OUT "link: $!";
-$fi->poll;
-is_deeply($hit, ["$tmpdir/new/link"], 'link(2) detected');
+@events = map { $_->fullname } $fi->read;
+is_deeply(\@events, ["$tmpdir/new/link"], 'link(2) detected');
 
 $w->cancel;
-@$hit = ();
 select undef, undef, undef, $MIN_FS_TICK;
 link("$tmpdir/new/tst", "$tmpdir/new/link2") or BAIL_OUT "link: $!";
-$fi->poll;
-is_deeply($hit, [], 'link(2) not detected after cancel');
+@events = map { $_->fullname } $fi->read;
+is_deeply(\@events, [], 'link(2) not detected after cancel');
 
 PublicInbox::DS->Reset;
 
diff --git a/t/kqnotify.t b/t/kqnotify.t
index b3414b8ae33..c3557d3e25b 100644
--- a/t/kqnotify.t
+++ b/t/kqnotify.t
@@ -17,25 +17,21 @@ close $fh or BAIL_OUT "close: $!";
 
 my $kqn = PublicInbox::KQNotify->new;
 my $mask = PublicInbox::KQNotify::MOVED_TO_OR_CREATE();
-my $hit = [];
-my $cb = sub { push @$hit, map { $_->fullname } @_ };
-my $w = $kqn->watch("$tmpdir/new", $mask, $cb);
+my $w = $kqn->watch("$tmpdir/new", $mask);
 
 rename("$tmpdir/tst", "$tmpdir/new/tst") or BAIL_OUT "rename: $!";
-$kqn->poll;
+my $hit = [ map { $_->fullname } $kqn->read ];
 is_deeply($hit, ["$tmpdir/new/tst"], 'rename(2) detected (via NOTE_EXTEND)');
 
-@$hit = ();
 open $fh, '>', "$tmpdir/tst" or BAIL_OUT "open: $!";
 close $fh or BAIL_OUT "close: $!";
 link("$tmpdir/tst", "$tmpdir/new/link") or BAIL_OUT "link: $!";
-$kqn->poll;
+$hit = [ grep m!/link$!, map { $_->fullname } $kqn->read ];
 is_deeply($hit, ["$tmpdir/new/link"], 'link(2) detected (via NOTE_WRITE)');
 
 $w->cancel;
-@$hit = ();
 link("$tmpdir/new/tst", "$tmpdir/new/link2") or BAIL_OUT "link: $!";
-$kqn->poll;
+$hit = [ map { $_->fullname } $kqn->read ];
 is_deeply($hit, [], 'link(2) not detected after cancel');
 
 done_testing;

Re: [PATCH] inboxidle: avoid per-inbox anonymous subs

[Eric Wong]

Keeping comments and code synced is hard :x
Will squash before pushing

diff --git a/lib/PublicInbox/DirIdle.pm b/lib/PublicInbox/DirIdle.pm
index 2915fd6e..89cce305 100644
--- a/lib/PublicInbox/DirIdle.pm
+++ b/lib/PublicInbox/DirIdle.pm
@@ -45,7 +45,7 @@ sub event_step {
 	my ($self) = @_;
 	my $cb = $self->{cb};
 	eval {
-		my @events = $self->{inot}->read; # Linux::Inotify2::poll
+		my @events = $self->{inot}->read; # Linux::Inotify2->read
 		$cb->($_) for @events;
 	};
 	warn "$self->{inot}->read err: $@\n" if $@;
diff --git a/lib/PublicInbox/KQNotify.pm b/lib/PublicInbox/KQNotify.pm
index 2305daa7..c7740df2 100644
--- a/lib/PublicInbox/KQNotify.pm
+++ b/lib/PublicInbox/KQNotify.pm
@@ -56,7 +56,7 @@ sub on_overflow {}
 # noop for Linux::Inotify2 compatibility, we use `0' timeout for ->kevent
 sub blocking {}
 
-# behave like Linux::Inotify2::poll
+# behave like Linux::Inotify2->read
 sub read {
 	my ($self) = @_;
 	my @kevents = $self->{dskq}->{kq}->kevent(0);