From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: X-Spam-Status: No, score=-4.0 required=3.0 tests=ALL_TRUSTED,BAYES_00 shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from localhost (dcvr.yhbt.net [127.0.0.1]) by dcvr.yhbt.net (Postfix) with ESMTP id C2D7B1F461 for ; Wed, 15 May 2019 10:57:24 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH] remove hard Devel::Peek dependency and lazy load for daemons Date: Wed, 15 May 2019 10:57:24 +0000 Message-Id: <20190515105724.26818-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: It's only useful for a corner case in long-running daemons when an admin decides to compact or vacuum a Xapian or SQLite DB. As a result, other scripts should run slightly faster. For instance, this saves about 80ms (2.710s => 2.630s) in t/mda.t on my remote workstation. While we're at it, make sure EvCleanup is properly require'd in Daemon.pm and HTTP.pm and document our use of Devel::Peek. --- INSTALL | 11 +++++----- Makefile.PL | 4 ---- lib/PublicInbox/Daemon.pm | 3 ++- lib/PublicInbox/HTTP.pm | 1 + lib/PublicInbox/Inbox.pm | 45 ++++++++++++++++++++++++++++----------- t/config_limiter.t | 1 - 6 files changed, 41 insertions(+), 24 deletions(-) diff --git a/INSTALL b/INSTALL index 313a295..8343762 100644 --- a/INSTALL +++ b/INSTALL @@ -36,11 +36,6 @@ Beyond that, there is a long list of Perl modules required, starting with: pkg: p5-TimeDate rpm: perl-TimeDate -* Devel::Peek deb: libperl5.$MINOR (e.g. libperl5.24) - pkg: perl5 - rpm: perl-Devel-Peek - (typically installed alongside Perl5) - * Email::MIME deb: libemail-mime-perl pkg: p5-Email-MIME rpm: perl-Email-MIME @@ -125,6 +120,12 @@ above, so there is no need to explicitly install them: rpm: perl-DBI (pulled in by DBD::SQLite) +* Devel::Peek deb: libperl5.$MINOR (e.g. libperl5.24) + pkg: perl5 + rpm: perl-Devel-Peek + (optional for stale FD cleanup in daemons, + typically installed alongside Perl5) + - Filesys::Notify::Simple deb: libfilesys-notify-simple-perl pkg: pkg-Filesys-Notify-Simple rpm: perl-Filesys-Notify-Simple diff --git a/Makefile.PL b/Makefile.PL index 6be913b..de0c49f 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -38,10 +38,6 @@ WriteMakefile( # `perl5' on FreeBSD 'Encode' => 0, - # libperl$PERL_VERSION on Debian, `perl5' on FreeBSD, - # but Fedora seems to need this separately - 'Devel::Peek' => 0, - # TODO: these should really be made optional... 'Plack' => 0, 'URI::Escape' => 0, diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm index 68ba987..227ba5f 100644 --- a/lib/PublicInbox/Daemon.pm +++ b/lib/PublicInbox/Daemon.pm @@ -13,6 +13,7 @@ use Time::HiRes qw(clock_gettime CLOCK_MONOTONIC); STDOUT->autoflush(1); STDERR->autoflush(1); require PublicInbox::DS; +require PublicInbox::EvCleanup; require POSIX; require PublicInbox::Listener; require PublicInbox::ParentPipe; @@ -463,6 +464,7 @@ sub master_loop { sub daemon_loop ($$) { my ($refresh, $post_accept) = @_; + PublicInbox::EvCleanup::enable(); # early for $refresh my $parent_pipe; if ($worker_processes > 0) { $refresh->(); # preload by default @@ -485,7 +487,6 @@ sub daemon_loop ($$) { @listeners = map { PublicInbox::Listener->new($_, $post_accept) } @listeners; - PublicInbox::EvCleanup::enable(); PublicInbox::DS->EventLoop; $parent_pipe = undef; } diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm index 11bd241..10e6d6a 100644 --- a/lib/PublicInbox/HTTP.pm +++ b/lib/PublicInbox/HTTP.pm @@ -18,6 +18,7 @@ use Plack::HTTPParser qw(parse_http_request); # XS or pure Perl use HTTP::Status qw(status_message); use HTTP::Date qw(time2str); use IO::Handle; +require PublicInbox::EvCleanup; use constant { CHUNK_START => -1, # [a-f0-9]+\r\n CHUNK_END => -2, # \r\n diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm index 76aeefb..0b118b2 100644 --- a/lib/PublicInbox/Inbox.pm +++ b/lib/PublicInbox/Inbox.pm @@ -7,25 +7,30 @@ use strict; use warnings; use PublicInbox::Git; use PublicInbox::MID qw(mid2path); -use Devel::Peek qw(SvREFCNT); use PublicInbox::MIME; +# Long-running "git-cat-file --batch" processes won't notice +# unlinked packs, so we need to restart those processes occasionally. +# Xapian and SQLite file handles are mostly stable, but sometimes an +# admin will attempt to replace them atomically after compact/vacuum +# and we need to be prepared for that. my $cleanup_timer; -eval { - $cleanup_timer = 'disabled'; - require PublicInbox::EvCleanup; - $cleanup_timer = undef; # OK if we get here -}; -my $cleanup_broken = $@; - +my $cleanup_avail = -1; # 0, or 1 +my $have_devel_peek; my $CLEANUP = {}; # string(inbox) -> inbox sub cleanup_task () { $cleanup_timer = undef; my $next = {}; for my $ibx (values %$CLEANUP) { my $again; - foreach my $f (qw(mm search)) { - delete $ibx->{$f} if SvREFCNT($ibx->{$f}) == 1; + if ($have_devel_peek) { + foreach my $f (qw(mm search)) { + # we bump refcnt by assigning tmp, here: + my $tmp = $ibx->{$f} or next; + next if Devel::Peek::SvREFCNT($tmp) > 2; + delete $ibx->{$f}; + # refcnt is zero when tmp is out-of-scope + } } my $expire = time - 60; if (my $git = $ibx->{git}) { @@ -36,16 +41,30 @@ sub cleanup_task () { $again = 1 if $git->cleanup($expire); } } - $again ||= !!($ibx->{mm} || $ibx->{search}); + if ($have_devel_peek) { + $again ||= !!($ibx->{mm} || $ibx->{search}); + } $next->{"$ibx"} = $ibx if $again; } $CLEANUP = $next; } +sub cleanup_possible () { + # no need to require EvCleanup, here, if it were enabled another + # module would've require'd it, already + eval { PublicInbox::EvCleanup::enabled() } or return 0; + + eval { + require Devel::Peek; # needs separate package in Fedora + $have_devel_peek = 1; + }; + 1; +} + sub _cleanup_later ($) { my ($self) = @_; - return if $cleanup_broken; - return unless PublicInbox::EvCleanup::enabled(); + $cleanup_avail = cleanup_possible() if $cleanup_avail < 0; + return if $cleanup_avail != 1; $cleanup_timer ||= PublicInbox::EvCleanup::later(*cleanup_task); $CLEANUP->{"$self"} = $self; } diff --git a/t/config_limiter.t b/t/config_limiter.t index 85a7125..b18951a 100644 --- a/t/config_limiter.t +++ b/t/config_limiter.t @@ -38,7 +38,6 @@ my $cfgpfx = "publicinbox.test"; ok($lim, 'Limiter exists'); is($lim->{max}, 3, 'limiter has expected slots'); $ibx->{git} = undef; - PublicInbox::Inbox::cleanup_task; my $new = $ibx->git; isnt($old, "$new", 'got new Git object'); is("$new->{-httpbackend_limiter}", "$lim", 'same limiter'); -- EW