[PATCH 0/7] psgi: more memory reductions

[PATCH 0/7] psgi: more memory reductions

[Eric Wong]

While of these are as significant as the patch avoid inadvertant
MIME objects storage in threads(*), they add up to some meaningful
reductions and can make it easier for memory-starved VPS to serve
serve public-inboxes.

I've diffed output of /T/, /t/ and &x=t endpoints of various HTML
pages before and after without finding differences.

There's definitely more that can be done in this area, though...

Sprinkling Devel::Size::total_size calls in various places (mostly
->getline iterators/callbacks ) was instrumental in the development
of these patches.

(*) https://public-inbox.org/meta/20190108004606.23760-1-e@80x24.org/
    ("view: stop storing all MIME objects on large threads")

Eric Wong (7):
  httpd: remove psgix.harakiri reference
  searchmsg: get rid of termlist scanning for mid
  searchmsg: remove Xapian::Document field
  searchview: drop unused {seen} hashref
  searchmsg: remove unused fields for PSGI in Xapian results
  over: cull unneeded fields for get_thread
  view: more culling for search threads

 lib/PublicInbox/HTTPD.pm        |  1 -
 lib/PublicInbox/Inbox.pm        |  5 ++--
 lib/PublicInbox/Over.pm         | 19 ++++++++-----
 lib/PublicInbox/SearchIdx.pm    |  6 ++--
 lib/PublicInbox/SearchMsg.pm    | 49 ++++++++++++++++-----------------
 lib/PublicInbox/SearchThread.pm |  5 ++++
 lib/PublicInbox/SearchView.pm   |  1 -
 lib/PublicInbox/View.pm         | 10 +++++--
 t/search.t                      | 10 ++++---
 9 files changed, 60 insertions(+), 46 deletions(-)

-- 
EW

[PATCH 1/7] httpd: remove psgix.harakiri reference

[Eric Wong]

We don't need to set "psgix." extension fields for things
we don't support.  This saves 138 bytes per-client in $env
as measured by Devel::Size::total_size
---
 lib/PublicInbox/HTTPD.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/PublicInbox/HTTPD.pm b/lib/PublicInbox/HTTPD.pm
index f923dbb..905afbb 100644
--- a/lib/PublicInbox/HTTPD.pm
+++ b/lib/PublicInbox/HTTPD.pm
@@ -27,7 +27,6 @@ sub new {
 		'psgi.run_once'	 => Plack::Util::FALSE,
 		'psgi.multithread' => Plack::Util::FALSE,
 		'psgi.multiprocess' => Plack::Util::TRUE,
-		'psgix.harakiri'=> Plack::Util::FALSE,
 		'psgix.input.buffered' => Plack::Util::TRUE,
 
 		# XXX unstable API!
-- 
EW

[PATCH 2/7] searchmsg: get rid of termlist scanning for mid

[Eric Wong]

It doesn't seem to be used anywhere
---
 lib/PublicInbox/SearchMsg.pm | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/SearchMsg.pm b/lib/PublicInbox/SearchMsg.pm
index c7787ea..5a2ca83 100644
--- a/lib/PublicInbox/SearchMsg.pm
+++ b/lib/PublicInbox/SearchMsg.pm
@@ -157,29 +157,17 @@ sub references {
 	defined $x ? $x : '';
 }
 
-sub _get_term_val ($$$) {
-	my ($self, $pfx, $re) = @_;
-	my $doc = $self->{doc};
-	my $end = $doc->termlist_end;
-	my $i = $doc->termlist_begin;
-	$i->skip_to($pfx);
-	if ($i != $end) {
-		my $val = $i->get_termname;
-		$val =~ s/$re// and return $val;
-	}
-	undef;
-}
-
 sub mid ($;$) {
 	my ($self, $mid) = @_;
 
 	if (defined $mid) {
 		$self->{mid} = $mid;
-	} elsif (my $rv = $self->{mid}) {
+	} elsif (defined(my $rv = $self->{mid})) {
 		$rv;
 	} elsif ($self->{doc}) {
-		$self->{mid} = _get_term_val($self, 'Q', qr/\AQ/);
+		die "SHOULD NOT HAPPEN\n";
 	} else {
+		die "NO {mime} for mid\n" unless $self->{mime};
 		$self->_extract_mid; # v1 w/o Xapian
 	}
 }
-- 
EW

[PATCH 3/7] searchmsg: remove Xapian::Document field

[Eric Wong]

We don't need to be carrying this around with the many SearchMsg
objects we have.  This saves about 20K from a large SearchView
"&x=t" response.
---
 lib/PublicInbox/SearchIdx.pm |  6 +++---
 lib/PublicInbox/SearchMsg.pm | 19 +++++++------------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/SearchIdx.pm b/lib/PublicInbox/SearchIdx.pm
index fd48169..7a8ebf3 100644
--- a/lib/PublicInbox/SearchIdx.pm
+++ b/lib/PublicInbox/SearchIdx.pm
@@ -282,7 +282,7 @@ sub index_body ($$$) {
 sub add_xapian ($$$$$) {
 	my ($self, $mime, $num, $oid, $mids, $mid0) = @_;
 	my $smsg = PublicInbox::SearchMsg->new($mime);
-	my $doc = $smsg->{doc};
+	my $doc = Search::Xapian::Document->new;
 	my $subj = $smsg->subject;
 	add_val($doc, PublicInbox::Search::TS(), $smsg->ts);
 	my @ds = gmtime($smsg->ds);
@@ -439,8 +439,8 @@ sub remove_by_oid {
 	for (; $head != $tail; $head->inc) {
 		my $docid = $head->get_docid;
 		my $doc = $db->get_document($docid);
-		my $smsg = PublicInbox::SearchMsg->wrap($doc, $mid);
-		$smsg->load_expand;
+		my $smsg = PublicInbox::SearchMsg->wrap($mid);
+		$smsg->load_expand($doc);
 		if ($smsg->{blob} eq $oid) {
 			push(@delete, $docid);
 		}
diff --git a/lib/PublicInbox/SearchMsg.pm b/lib/PublicInbox/SearchMsg.pm
index 5a2ca83..65e085f 100644
--- a/lib/PublicInbox/SearchMsg.pm
+++ b/lib/PublicInbox/SearchMsg.pm
@@ -13,20 +13,18 @@ use Time::Local qw(timegm);
 
 sub new {
 	my ($class, $mime) = @_;
-	my $doc = Search::Xapian::Document->new;
-	bless { doc => $doc, mime => $mime }, $class;
+	bless { mime => $mime }, $class;
 }
 
 sub wrap {
-	my ($class, $doc, $mid) = @_;
-	bless { doc => $doc, mime => undef, mid => $mid }, $class;
+	my ($class, $mid) = @_;
+	bless { mid => $mid }, $class;
 }
 
 sub get {
 	my ($class, $head, $db, $mid) = @_;
 	my $doc_id = $head->get_docid;
-	my $doc = $db->get_document($doc_id);
-	load_expand(wrap($class, $doc, $mid))
+	load_expand(wrap($class, $mid), $db->get_document($doc_id));
 }
 
 sub get_val ($$) {
@@ -70,8 +68,7 @@ sub load_from_data ($$) {
 }
 
 sub load_expand {
-	my ($self) = @_;
-	my $doc = $self->{doc};
+	my ($self, $doc) = @_;
 	my $data = $doc->get_data or return;
 	$self->{ts} = get_val($doc, PublicInbox::Search::TS());
 	my $dt = get_val($doc, PublicInbox::Search::DT());
@@ -84,8 +81,8 @@ sub load_expand {
 
 sub load_doc {
 	my ($class, $doc) = @_;
-	my $self = bless { doc => $doc }, $class;
-	$self->load_expand;
+	my $self = bless {}, $class;
+	load_expand($self, $doc);
 }
 
 # :bytes and :lines metadata in RFC 3977
@@ -164,8 +161,6 @@ sub mid ($;$) {
 		$self->{mid} = $mid;
 	} elsif (defined(my $rv = $self->{mid})) {
 		$rv;
-	} elsif ($self->{doc}) {
-		die "SHOULD NOT HAPPEN\n";
 	} else {
 		die "NO {mime} for mid\n" unless $self->{mime};
 		$self->_extract_mid; # v1 w/o Xapian
-- 
EW

[PATCH 4/7] searchview: drop unused {seen} hashref

[Eric Wong]

Unused since commit 5f09452bb7e6cf49fb6eb7e6cf166a7c3cdc5433
("view: cull redundant phrases in subjects")
---
 lib/PublicInbox/SearchView.pm | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/PublicInbox/SearchView.pm b/lib/PublicInbox/SearchView.pm
index e47dcfd..c33caec 100644
--- a/lib/PublicInbox/SearchView.pm
+++ b/lib/PublicInbox/SearchView.pm
@@ -240,7 +240,6 @@ sub mset_thread {
 	$ctx->{pct} = \%pct;
 	$ctx->{prev_attr} = '';
 	$ctx->{prev_level} = 0;
-	$ctx->{seen} = {};
 	$ctx->{s_nr} = scalar(@$msgs).'+ results';
 
 	# reduce hash lookups in skel_dump
-- 
EW

[PATCH 5/7] searchmsg: remove unused fields for PSGI in Xapian results

[Eric Wong]

These fields are only necessary in NNTP and not even stored in
Xapian; so keeping them around for the PSGI web UI search
results wastes nearly 80K when loading large result sets.
---
 lib/PublicInbox/SearchMsg.pm | 13 ++++++++++++-
 t/search.t                   | 10 ++++++----
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/lib/PublicInbox/SearchMsg.pm b/lib/PublicInbox/SearchMsg.pm
index 65e085f..292efce 100644
--- a/lib/PublicInbox/SearchMsg.pm
+++ b/lib/PublicInbox/SearchMsg.pm
@@ -57,11 +57,14 @@ sub load_from_data ($$) {
 
 		# To: and Cc: are stored to optimize HDR/XHDR in NNTP since
 		# some NNTP clients will use that for message displays.
+		# NNTP only, and only stored in Over(view), not Xapian
 		$self->{to},
 		$self->{cc},
 
 		$self->{blob},
 		$self->{mid},
+
+		# NNTP only
 		$self->{bytes},
 		$self->{lines}
 	) = split(/\n/, $_[1]);
@@ -79,10 +82,18 @@ sub load_expand {
 	$self;
 }
 
+# Only called by PSGI interface, not NNTP
 sub load_doc {
 	my ($class, $doc) = @_;
 	my $self = bless {}, $class;
-	load_expand($self, $doc);
+	my $smsg = load_expand($self, $doc);
+
+	from_name($smsg); # fill in {from_name} so we can delete {from}
+
+	# drop NNTP-only fields which aren't relevant to PSGI results:
+	# saves ~80K on a 200 item search result:
+	delete @$smsg{qw(from ts to cc bytes lines)};
+	$smsg;
 }
 
 # :bytes and :lines metadata in RFC 3977
diff --git a/t/search.t b/t/search.t
index 3c758e6..6415a64 100644
--- a/t/search.t
+++ b/t/search.t
@@ -341,7 +341,7 @@ $ibx->with_umask(sub {
 		is(scalar(@$res), 1,
 			"searched $pfx successfully for From:");
 		foreach my $smsg (@$res) {
-			like($smsg->from, qr/Laggy Sender/,
+			like($smsg->from_name, qr/Laggy Sender/,
 				"From appears with $pfx");
 		}
 	}
@@ -358,16 +358,18 @@ $ibx->with_umask(sub {
 
 	$res = $ro->query('q:theatre');
 	is(scalar(@$res), 1, 'only one quoted body');
-	like($res->[0]->from, qr/\AQuoter/, 'got quoted body') if scalar(@$res);
+	like($res->[0]->from_name, qr/\AQuoter/,
+		'got quoted body') if (scalar(@$res));
 
 	$res = $ro->query('nq:theatre');
 	is(scalar @$res, 1, 'only one non-quoted body');
-	like($res->[0]->from, qr/\ANon-Quoter/, 'got non-quoted body') if scalar(@$res);
+	like($res->[0]->from_name, qr/\ANon-Quoter/,
+		'got non-quoted body') if (scalar(@$res));
 
 	foreach my $pfx (qw(b: bs:)) {
 		$res = $ro->query($pfx . 'theatre');
 		is(scalar @$res, 2, "searched both bodies for $pfx");
-		like($res->[0]->from, qr/\ANon-Quoter/,
+		like($res->[0]->from_name, qr/\ANon-Quoter/,
 			"non-quoter first for $pfx") if scalar(@$res);
 	}
 }
-- 
EW

[PATCH 6/7] over: cull unneeded fields for get_thread

[Eric Wong]

On a certain ugly /$INBOX/$MESSAGE_ID/T/ endpoint with 1000
messages in the thread, this cuts memory usage from 2.5M to 1.9M
(which still isn't great, but it's a start).
---
 lib/PublicInbox/Over.pm      | 19 ++++++++++++-------
 lib/PublicInbox/SearchMsg.pm | 19 +++++++++++--------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/lib/PublicInbox/Over.pm b/lib/PublicInbox/Over.pm
index dda1e6d..598c9fc 100644
--- a/lib/PublicInbox/Over.pm
+++ b/lib/PublicInbox/Over.pm
@@ -40,13 +40,16 @@ sub disconnect { $_[0]->{dbh} = undef }
 
 sub connect { $_[0]->{dbh} ||= $_[0]->dbh_new }
 
-sub load_from_row {
-	my ($smsg) = @_;
+sub load_from_row ($;$) {
+	my ($smsg, $cull) = @_;
 	bless $smsg, 'PublicInbox::SearchMsg';
 	if (defined(my $data = delete $smsg->{ddd})) {
 		$data = uncompress($data);
 		utf8::decode($data);
-		$smsg->load_from_data($data);
+		PublicInbox::SearchMsg::load_from_data($smsg, $data);
+
+		# saves over 600K for 1000+ message threads
+		PublicInbox::SearchMsg::psgi_cull($smsg) if $cull;
 	}
 	$smsg
 }
@@ -57,7 +60,8 @@ sub do_get {
 	my $lim = (($opts->{limit} || 0) + 0) || DEFAULT_LIMIT;
 	$sql .= "LIMIT $lim";
 	my $msgs = $dbh->selectall_arrayref($sql, { Slice => {} }, @args);
-	load_from_row($_) for @$msgs;
+	my $cull = $opts->{cull};
+	load_from_row($_, $cull) for @$msgs;
 	$msgs
 }
 
@@ -82,6 +86,7 @@ sub nothing () { wantarray ? (0, []) : [] };
 sub get_thread {
 	my ($self, $mid, $prev) = @_;
 	my $dbh = $self->connect;
+	my $opts = { cull => 1 };
 
 	my $id = $dbh->selectrow_array(<<'', undef, $mid);
 SELECT id FROM msgid WHERE mid = ? LIMIT 1
@@ -109,7 +114,7 @@ SELECT tid,sid FROM over WHERE num = ? LIMIT 1
 
 	my $cols = 'num,ts,ds,ddd';
 	unless (wantarray) {
-		return do_get($self, <<"", {}, $tid, $sid, $num);
+		return do_get($self, <<"", $opts, $tid, $sid, $num);
 SELECT $cols FROM over WHERE $cond_all
 ORDER BY $sort_col ASC
 
@@ -123,14 +128,14 @@ SELECT COUNT(num) FROM over WHERE $cond_all
 
 	# giant thread, prioritize strict (tid) matches and throw
 	# in the loose (sid) matches at the end
-	my $msgs = do_get($self, <<"", {}, $tid, $num);
+	my $msgs = do_get($self, <<"", $opts, $tid, $num);
 SELECT $cols FROM over WHERE tid = ? AND num > ?
 ORDER BY $sort_col ASC
 
 	# do we have room for loose matches? get the most recent ones, first:
 	my $lim = DEFAULT_LIMIT - scalar(@$msgs);
 	if ($lim > 0) {
-		my $opts = { limit => $lim };
+		$opts->{limit} = $lim;
 		my $loose = do_get($self, <<"", $opts, $tid, $sid, $num);
 SELECT $cols FROM over WHERE tid != ? AND sid = ? AND num > ?
 ORDER BY $sort_col DESC
diff --git a/lib/PublicInbox/SearchMsg.pm b/lib/PublicInbox/SearchMsg.pm
index 292efce..722a1b9 100644
--- a/lib/PublicInbox/SearchMsg.pm
+++ b/lib/PublicInbox/SearchMsg.pm
@@ -82,18 +82,21 @@ sub load_expand {
 	$self;
 }
 
+sub psgi_cull ($) {
+	my ($self) = @_;
+	from_name($self); # fill in {from_name} so we can delete {from}
+
+	# drop NNTP-only fields which aren't relevant to PSGI results:
+	# saves ~80K on a 200 item search result:
+	delete @$self{qw(from ts to cc bytes lines)};
+	$self;
+}
+
 # Only called by PSGI interface, not NNTP
 sub load_doc {
 	my ($class, $doc) = @_;
 	my $self = bless {}, $class;
-	my $smsg = load_expand($self, $doc);
-
-	from_name($smsg); # fill in {from_name} so we can delete {from}
-
-	# drop NNTP-only fields which aren't relevant to PSGI results:
-	# saves ~80K on a 200 item search result:
-	delete @$smsg{qw(from ts to cc bytes lines)};
-	$smsg;
+	psgi_cull(load_expand($self, $doc));
 }
 
 # :bytes and :lines metadata in RFC 3977
-- 
EW

[PATCH 7/7] view: more culling for search threads

[Eric Wong]

{mapping} overhead is now down to ~1.3M at the end of
a giant thread from hell.
---
 lib/PublicInbox/Inbox.pm        |  5 +++--
 lib/PublicInbox/SearchThread.pm |  5 +++++
 lib/PublicInbox/View.pm         | 10 ++++++++--
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/lib/PublicInbox/Inbox.pm b/lib/PublicInbox/Inbox.pm
index 73f5761..d57e46d 100644
--- a/lib/PublicInbox/Inbox.pm
+++ b/lib/PublicInbox/Inbox.pm
@@ -302,8 +302,9 @@ sub smsg_by_mid ($$) {
 	my ($self, $mid) = @_;
 	my $srch = search($self) or return;
 	# favor the Message-ID we used for the NNTP article number:
-	my $num = mid2num($self, $mid);
-	defined $num ? $srch->lookup_article($num) : undef;
+	defined(my $num = mid2num($self, $mid)) or return;
+	my $smsg = $srch->lookup_article($num) or return;
+	PublicInbox::SearchMsg::psgi_cull($smsg);
 }
 
 sub msg_by_mid ($$;$) {
diff --git a/lib/PublicInbox/SearchThread.pm b/lib/PublicInbox/SearchThread.pm
index be29098..931bd57 100644
--- a/lib/PublicInbox/SearchThread.pm
+++ b/lib/PublicInbox/SearchThread.pm
@@ -53,6 +53,11 @@ sub _add_message ($$) {
 	my $this = _get_cont_for_id($id_table, $smsg->{mid});
 	$this->{smsg} = $smsg;
 
+	# saves around 4K across 1K messages
+	# TODO: move this to a more appropriate place, breaks tests
+	# if we do it during psgi_cull
+	delete $smsg->{num};
+
 	# B. For each element in the message's References field:
 	defined(my $refs = $smsg->{references}) or return;
 
diff --git a/lib/PublicInbox/View.pm b/lib/PublicInbox/View.pm
index 5ddb842..cd125e0 100644
--- a/lib/PublicInbox/View.pm
+++ b/lib/PublicInbox/View.pm
@@ -219,7 +219,10 @@ sub index_entry {
 	$rv .= _th_index_lite($mid_raw, \$irt, $id, $ctx);
 	my @tocc;
 	my $ds = $smsg->ds; # for v1 non-Xapian/SQLite users
-	my $mime = delete $smsg->{mime}; # critical to memory use
+	# deleting {mime} is critical to memory use,
+	# the rest of the fields saves about 400K as we iterate across 1K msgs
+	my ($mime) = delete @$smsg{qw(mime ds ts blob subject)};
+
 	my $hdr = $mime->header_obj;
 	my $from = _hdr_names_html($hdr, 'From');
 	obfuscate_addrs($obfs_ibx, $from) if $obfs_ibx;
@@ -311,7 +314,10 @@ sub _th_index_lite {
 	my $nr_s = 0;
 	my $siblings;
 	if (my $smsg = $node->{smsg}) {
-		($$irt) = (($smsg->{references} || '') =~ m/<([^>]+)>\z/);
+		# delete saves about 200KB on a 1K message thread
+		if (my $refs = delete $smsg->{references}) {
+			($$irt) = ($refs =~ m/<([^>]+)>\z/);
+		}
 	}
 	my $irt_map = $mapping->{$$irt} if defined $$irt;
 	if (defined $irt_map) {
-- 
EW