[PATCH 0/4] xap_helper: -imapd support + various updates

[PATCH 0/4] xap_helper: -imapd support + various updates

[Eric Wong]

Another step towards making everything in -netd non-blocking
wrt. potentially expensive Xapian queries.

Still pondering different ways to go about lei integration since
I don't like having lingering processes for lei...
Once again, public-facing daemons are completely different from
lei in design philosophy...

Eric Wong (4):
  search: remove xhc_start_maybe
  daemon: slightly simplify xap_helper spawning
  imap: support external xap_helper process
  t/xap_helper: bail on build failure if TEST_XH_CXX_ONLY

 lib/PublicInbox/Daemon.pm       | 10 ++++----
 lib/PublicInbox/IMAP.pm         | 19 +++++++++++---
 lib/PublicInbox/Search.pm       | 12 ++++-----
 lib/PublicInbox/XapHelper.pm    |  2 ++
 lib/PublicInbox/XapHelperCxx.pm |  1 +
 lib/PublicInbox/xap_helper.h    | 44 ++++++++++++++++-----------------
 t/xap_helper.t                  |  5 +++-
 7 files changed, 53 insertions(+), 40 deletions(-)

[PATCH 1/4] search: remove xhc_start_maybe

[Eric Wong]

We start xap_helper processes early in both public-facing
daemons to ensure maximum sharing.  lei may be different
since lingering processes likely sit idle for long periods
and we don't want to tie up RAM or swap space for idle
processes.
---
 lib/PublicInbox/Search.pm | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/lib/PublicInbox/Search.pm b/lib/PublicInbox/Search.pm
index 0e288cf0..98ad5034 100644
--- a/lib/PublicInbox/Search.pm
+++ b/lib/PublicInbox/Search.pm
@@ -446,13 +446,6 @@ sub mset {
 	do_enquire($self, $qry, $opt, TS);
 }
 
-sub xhc_start_maybe (@) {
-	require PublicInbox::XapClient;
-	my $xhc = PublicInbox::XapClient::start_helper(@_);
-	require PublicInbox::XhcMset if $xhc;
-	$xhc;
-}
-
 my %QPMETHOD_2_SYM = (add_prefix => ':', add_boolean_prefix => '=');
 
 sub xh_opt ($$) {

[PATCH 2/4] daemon: slightly simplify xap_helper spawning

[Eric Wong]

Avoid typing the fully-qualified global $XHC variable twice in
spawn_xh.  We'll also delay loading of XhcMset until successful
spawn and improve the error message in case of error.  There's
also no need to conditionally local-ize
$PublicInbox::Search::XHC, so do it unconditionally to reduce
branches.
---
 lib/PublicInbox/Daemon.pm | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/Daemon.pm b/lib/PublicInbox/Daemon.pm
index e00e0a78..5d93f81f 100644
--- a/lib/PublicInbox/Daemon.pm
+++ b/lib/PublicInbox/Daemon.pm
@@ -390,19 +390,19 @@ sub worker_quit { # $_[0] = signal name or number (unused)
 
 sub spawn_xh () {
 	$xh_workers // return;
-	require PublicInbox::XhcMset;
 	local $) = $gid if defined $gid;
 	local $( = $gid if defined $gid;
 	local $> = $uid if defined $uid;
 	local $< = $uid if defined $uid;
-	$PublicInbox::Search::XHC = eval {
+	my $xhc = $PublicInbox::Search::XHC = eval {
 		local $ENV{STDERR_PATH} = $stderr;
 		local $ENV{STDOUT_PATH} = $stdout;
 		PublicInbox::XapClient::start_helper('-j', $xh_workers)
 	};
 	if ($@) {
-		warn "E: $@";
-	} elsif (my $xhc = $PublicInbox::Search::XHC) {
+		warn "E: $@ (will attempt to continue w/o xapian-helpers)\n";
+	} elsif ($xhc) {
+		require PublicInbox::XhcMset;
 		$xhc->{io}->blocking(0);
 		awaitpid($xhc->{io}->attached_pid, \&respawn_xh);
 	}
@@ -746,7 +746,7 @@ sub run {
 	local $SIG{__WARN__} = PublicInbox::Eml::warn_ignore_cb();
 	local %WORKER_SIG = %WORKER_SIG;
 	local $PublicInbox::XapClient::tries = 0;
-	local $PublicInbox::Search::XHC if defined($xh_workers);
+	local $PublicInbox::Search::XHC;
 
 	daemon_loop();
 	# $unlink_on_leave runs

[PATCH 3/4] imap: support external xap_helper process

[Eric Wong]

Moving Xapian requests to an external process prevents expensive
searches from affecting non-search IMAP commands or any other
non-search requests within a -netd process if IMAP is sharing a
process with other public-facing protocols.
---
 lib/PublicInbox/IMAP.pm         | 19 +++++++++++---
 lib/PublicInbox/Search.pm       |  5 ++++
 lib/PublicInbox/XapHelper.pm    |  2 ++
 lib/PublicInbox/XapHelperCxx.pm |  1 +
 lib/PublicInbox/xap_helper.h    | 44 ++++++++++++++++-----------------
 5 files changed, 44 insertions(+), 27 deletions(-)

diff --git a/lib/PublicInbox/IMAP.pm b/lib/PublicInbox/IMAP.pm
index 74249784..e72c646c 100644
--- a/lib/PublicInbox/IMAP.pm
+++ b/lib/PublicInbox/IMAP.pm
@@ -1091,6 +1091,18 @@ sub parse_imap_query ($$) {
 	$q;
 }
 
+sub mset_cb { # async_mset cb
+	my ($self, $tag, $want_msn, $srch, $opt, $mset, $err) = @_;
+	$self->write($err ? do {
+		warn "IMAP search error: $err\n";
+		\"$tag BAD program fault - command not performed\r\n"
+	} : do {
+		my $uids = $srch->mset_to_artnums($mset, $opt);
+		msn_convert($self, $uids) if scalar(@$uids) && $want_msn;
+		\"* SEARCH @$uids\r\n$tag OK Search done\r\n"
+	});
+}
+
 sub search_common {
 	my ($self, $tag, $query, $want_msn) = @_;
 	my $ibx = $self->{ibx} or return "$tag BAD No mailbox selected\r\n";
@@ -1109,10 +1121,9 @@ sub search_common {
 			limit => UID_SLICE,
 			uid_range => $range_info
 		};
-		my $mset = $srch->mset($q, $opt);
-		my $uids = $srch->mset_to_artnums($mset, $opt);
-		msn_convert($self, $uids) if scalar(@$uids) && $want_msn;
-		"* SEARCH @$uids\r\n$tag OK Search done\r\n";
+		$srch->async_mset($q, $opt, \&mset_cb,
+					$self, $tag, $want_msn, $srch, $opt) ?
+				undef : '';
 	} else {
 		"$tag BAD Error\r\n";
 	}
diff --git a/lib/PublicInbox/Search.pm b/lib/PublicInbox/Search.pm
index 98ad5034..52e1f335 100644
--- a/lib/PublicInbox/Search.pm
+++ b/lib/PublicInbox/Search.pm
@@ -92,10 +92,12 @@ our @XH_SPEC = (
 	'o=i', # offset
 	'r', # 1=relevance then column
 	't', # collapse threads
+	'u=i', # IMAP UID min
 	'A=s@', # prefixes
 	'K=i', # timeout kill after i seconds
 	'O=s', # eidx_key
 	'T=i', # threadid
+	'U=i', # IMAP UID max
 	'Q=s@', # query prefixes "$user_prefix[:=]$XPREFIX"
 );
 
@@ -471,6 +473,9 @@ sub xh_opt ($$) {
 	push @ret, '-t' if $opt->{threads};
 	push @ret, '-T', $opt->{threadid} if defined $opt->{threadid};
 	push @ret, '-O', $opt->{eidx_key} if defined $opt->{eidx_key};
+	if (my $uid = $opt->{uid_range}) {
+		push @ret, '-u', $uid->[0], '-U', $uid->[1];
+	}
 	@ret;
 }
 
diff --git a/lib/PublicInbox/XapHelper.pm b/lib/PublicInbox/XapHelper.pm
index 15abed79..d3c98f03 100644
--- a/lib/PublicInbox/XapHelper.pm
+++ b/lib/PublicInbox/XapHelper.pm
@@ -162,6 +162,8 @@ sub cmd_mset { # to be used by WWW + IMAP
 	$opt->{threads} = 1 if defined $req->{t};
 	$opt->{git_dir} = $req->{g} if defined $req->{g};
 	$opt->{eidx_key} = $req->{O} if defined $req->{O};
+	my @uid_range = @$req{qw(u U)};
+	$opt->{uid_range} = \@uid_range if grep(defined, @uid_range) == 2;
 	$opt->{threadid} = $req->{T} if defined $req->{T};
 	my $mset = $req->{srch}->mset($qry_str, $opt);
 	say { $req->{0} } 'mset.size=', $mset->size,
diff --git a/lib/PublicInbox/XapHelperCxx.pm b/lib/PublicInbox/XapHelperCxx.pm
index 817abcfb..47807f9b 100644
--- a/lib/PublicInbox/XapHelperCxx.pm
+++ b/lib/PublicInbox/XapHelperCxx.pm
@@ -35,6 +35,7 @@ my $ldflags = '-Wl,-O1';
 $ldflags .= ' -Wl,--compress-debug-sections=zlib' if $^O ne 'openbsd';
 my $xflags = ($ENV{CXXFLAGS} // '-Wall -ggdb3 -pipe') . ' ' .
 	' -DTHREADID=' . PublicInbox::Search::THREADID .
+	' -DUID=' . PublicInbox::Search::UID .
 	' -DSHARD_COST=' . PublicInbox::Search::SHARD_COST .
 	' -DXH_SPEC="'.join('',
 		map { s/=.*/:/; $_ } @PublicInbox::Search::XH_SPEC) . '" ' .
diff --git a/lib/PublicInbox/xap_helper.h b/lib/PublicInbox/xap_helper.h
index 9c8436dc..692765ca 100644
--- a/lib/PublicInbox/xap_helper.h
+++ b/lib/PublicInbox/xap_helper.h
@@ -185,6 +185,7 @@ struct req { // argv and pfxv point into global rbuf
 	unsigned long long max;
 	unsigned long long off;
 	unsigned long long threadid;
+	unsigned long long uid_min, uid_max;
 	unsigned long timeout_sec;
 	size_t nr_out;
 	long sort_col; // value column, negative means BoolWeight
@@ -277,7 +278,14 @@ static Xapian::MSet mail_mset(struct req *req, const char *qry_str)
 		qry = Xapian::Query(Xapian::Query::OP_FILTER, qry,
 					Xapian::Query(req->Oeidx_key));
 	}
-	// TODO: uid_range
+	// THREADID and UID are CPP macros defined by XapHelperCxx.pm
+	if (req->uid_min != ULLONG_MAX && req->uid_max != ULLONG_MAX) {
+		Xapian::Query range = Xapian::Query(
+				Xapian::Query::OP_VALUE_RANGE, UID,
+				Xapian::sortable_serialise(req->uid_min),
+				Xapian::sortable_serialise(req->uid_max));
+		qry = Xapian::Query(Xapian::Query::OP_FILTER, qry, range);
+	}
 	if (req->threadid != ULLONG_MAX) {
 		std::string tid = Xapian::sortable_serialise(req->threadid);
 		qry = Xapian::Query(Xapian::Query::OP_FILTER, qry,
@@ -286,7 +294,6 @@ static Xapian::MSet mail_mset(struct req *req, const char *qry_str)
 	}
 	Xapian::Enquire enq = prep_enquire(req);
 	enq.set_query(qry);
-	// THREADID is a CPP macro defined on CLI (see) XapHelperCxx.pm
 	if (req->collapse_threads && has_threadid(srch))
 		enq.set_collapse_key(THREADID);
 
@@ -675,6 +682,11 @@ static void srch_init_extra(struct req *req)
 	req->srch->qp_extra_done = true;
 }
 
+#define OPT_U(ch, var, fn, max) do { \
+	var = fn(optarg, &end, 10); \
+	if (*end || var == max) ABORT("-"#ch" %s", optarg); \
+} while (0)
+
 static void dispatch(struct req *req)
 {
 	int c;
@@ -685,7 +697,7 @@ static void dispatch(struct req *req)
 	} kbuf;
 	char *end;
 	FILE *kfp;
-	req->threadid = ULLONG_MAX;
+	req->threadid = req->uid_min = req->uid_max = ULLONG_MAX;
 	for (c = 0; c < (int)MY_ARRAY_SIZE(cmds); c++) {
 		if (cmds[c].fn_len == size &&
 			!memcmp(cmds[c].fn_name, req->argv[0], size)) {
@@ -723,34 +735,20 @@ static void dispatch(struct req *req)
 			case LONG_MAX: case LONG_MIN: ABORT("-k %s", optarg);
 			}
 			break;
-		case 'm':
-			req->max = strtoull(optarg, &end, 10);
-			if (*end || req->max == ULLONG_MAX)
-				ABORT("-m %s", optarg);
-			break;
-		case 'o':
-			req->off = strtoull(optarg, &end, 10);
-			if (*end || req->off == ULLONG_MAX)
-				ABORT("-o %s", optarg);
-			break;
+		case 'm': OPT_U(m, req->max, strtoull, ULLONG_MAX); break;
+		case 'o': OPT_U(o, req->off, strtoull, ULLONG_MAX); break;
 		case 'r': req->relevance = true; break;
 		case 't': req->collapse_threads = true; break;
+		case 'u': OPT_U(u, req->uid_min, strtoull, ULLONG_MAX); break;
 		case 'A':
 			req->pfxv[req->pfxc++] = optarg;
 			if (MY_ARG_MAX == req->pfxc)
 				ABORT("too many -A");
 			break;
-		case 'K':
-			req->timeout_sec = strtoul(optarg, &end, 10);
-			if (*end || req->timeout_sec == ULONG_MAX)
-				ABORT("-K %s", optarg);
-			break;
+		case 'K': OPT_U(K, req->timeout_sec, strtoul, ULONG_MAX); break;
 		case 'O': req->Oeidx_key = optarg - 1; break; // pad "O" prefix
-		case 'T':
-			req->threadid = strtoull(optarg, &end, 10);
-			if (*end || req->threadid == ULLONG_MAX)
-				ABORT("-T %s", optarg);
-			break;
+		case 'T': OPT_U(T, req->threadid, strtoull, ULLONG_MAX); break;
+		case 'U': OPT_U(U, req->uid_max, strtoull, ULLONG_MAX); break;
 		case 'Q':
 			req->qpfxv[req->qpfxc++] = optarg;
 			if (MY_ARG_MAX == req->qpfxc) ABORT("too many -Q");

[PATCH 4/4] t/xap_helper: bail on build failure if TEST_XH_CXX_ONLY

[Eric Wong]

We shouldn't transparently fall back to the Perl bindings test
on build failures if the tester wants to explicitly test the C++
implementation.
---
 t/xap_helper.t | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/xap_helper.t b/t/xap_helper.t
index e87c9da8..da26b9a8 100644
--- a/t/xap_helper.t
+++ b/t/xap_helper.t
@@ -189,7 +189,10 @@ SKIP: {
 		require PublicInbox::XapHelperCxx;
 		PublicInbox::XapHelperCxx::cmd();
 	};
-	skip "XapHelperCxx build: $@", 1 if $@;
+	if ($@) {
+		xbail "C++ build failed: $@" if $ENV{TEST_XH_CXX_ONLY};
+		skip "XapHelperCxx build: $@", 1;
+	}
 
 	@NO_CXX = $ENV{TEST_XH_CXX_ONLY} ? (0) : (0, 1);
 	my $ar = $test->(@$cmd, '-j0');