From 766cd51e9c77f74a2781ff35fed6554f08e0f29d Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 27 Jun 2019 04:11:22 +0000 Subject: nntp: rework and simplify art_lookup response We don't need some of the array elements returned from art_lookup, anymore (and haven't used them in years). We can also shorten the lifetime of the Email::Simple object by relying on the fact Email::Simple->new will modify it's arg if given a SCALARREF and allow us to avoid Email::Simple::body calls. Unfortunately, this doesn't seem to provide any noticeable improvement in memory usage when dealing with a 30+ MB test message, since our previous use of ->body_set('') was saving some memory, but forcing a LF-only body to be CRLF was making Perl allocate extra space for s///sg. --- lib/PublicInbox/NNTP.pm | 47 ++++++++++++++++++++++------------------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index 53e18281..30d3dab6 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -510,24 +510,21 @@ find_mid: found: my $smsg = $ng->over->get_art($n) or return $err; my $msg = $ng->msg_by_smsg($smsg) or return $err; - my $s = Email::Simple->new($msg); - if ($set_headers) { - set_nntp_headers($self, $s->header_obj, $ng, $n, $mid); - # must be last - $s->body_set('') if ($set_headers == 2); - } - [ $n, $mid, $s, $smsg->bytes, $smsg->lines, $ng ]; + # Email::Simple->new will modify $msg in-place as documented + # in its manpage, so what's left is the body and we won't need + # to call Email::Simple::body(), later + my $hdr = Email::Simple->new($msg)->header_obj; + set_nntp_headers($self, $hdr, $ng, $n, $mid) if $set_headers; + [ $n, $mid, $msg, $hdr ]; } -sub simple_body_write ($$) { - my ($self, $s) = @_; - my $body = $s->body; - $s->body_set(''); - $body =~ s/^\./../smg; - $body =~ s/(?header_obj->as_string; + my $hdr = $_[0]->as_string; utf8::encode($hdr); $hdr =~ s/(? article retrieved - head and body follow"); - msg_more($self, _header($s)); + msg_more($self, _header($hdr)); msg_more($self, "\r\n"); - simple_body_write($self, $s); + msg_body_write($self, $msg); } sub cmd_head ($;$) { my ($self, $art) = @_; my $r = art_lookup($self, $art, 2); return $r unless ref $r; - my ($n, $mid, $s) = @$r; + my ($n, $mid, undef, $hdr) = @$r; set_art($self, $art); more($self, "221 $n <$mid> article retrieved - head follows"); - msg_more($self, _header($s)); + msg_more($self, _header($hdr)); '.' } @@ -577,17 +574,17 @@ sub cmd_body ($;$) { my ($self, $art) = @_; my $r = art_lookup($self, $art, 0); return $r unless ref $r; - my ($n, $mid, $s) = @$r; + my ($n, $mid, $msg) = @$r; set_art($self, $art); more($self, "222 $n <$mid> article retrieved - body follows"); - simple_body_write($self, $s); + msg_body_write($self, $msg); } sub cmd_stat ($;$) { my ($self, $art) = @_; my $r = art_lookup($self, $art, 0); return $r unless ref $r; - my ($n, $mid, undef) = @$r; + my ($n, $mid) = @$r; set_art($self, $art); "223 $n <$mid> article retrieved - request text separately"; } @@ -815,7 +812,7 @@ sub hdr_mid_prefix ($$$$$) { } sub hdr_mid_response ($$$$$$) { - my ($self, $xhdr, $ng, $n, $mid, $v) = @_; # r: art_lookup result + my ($self, $xhdr, $ng, $n, $mid, $v) = @_; my $res = ''; if ($xhdr) { $res .= r221 . "\r\n"; -- cgit v1.2.3-24-ge0c7 From 018295ad147ecbe9759d46bc8d74a776d4ad382f Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 27 Jun 2019 08:40:19 +0000 Subject: mbox: use Email::Simple->new to do in-place modifications Email::Simple->new will split the head from the body in-place, and we can avoid using Email::Simple::body. This saves us from holding an extra copy of the message in memory, and saves us around ~30MB when operating on ~30MB messages. --- lib/PublicInbox/Mbox.pm | 38 +++++++++++++++++++------------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/lib/PublicInbox/Mbox.pm b/lib/PublicInbox/Mbox.pm index 15200d3a..1bf71c60 100644 --- a/lib/PublicInbox/Mbox.pm +++ b/lib/PublicInbox/Mbox.pm @@ -38,17 +38,18 @@ sub mb_stream { # called by PSGI server as body response sub getline { my ($more) = @_; # self - my ($ctx, $id, $prev, $next, $cur) = @$more; - if ($cur) { # first + my ($ctx, $id, $prev, $next, $cur, $mref) = @$more; + if ($mref) { # first pop @$more; - return msg_str($ctx, $cur); + pop @$more; + return msg_str($ctx, $cur, $mref); } $cur = $next or return; my $ibx = $ctx->{-inbox}; $next = $ibx->over->next_by_mid($ctx->{mid}, \$id, \$prev); @$more = ($ctx, $id, $prev, $next); # $next may be undef, here - my $mref = $ibx->msg_by_smsg($cur) or return; - msg_str($ctx, Email::Simple->new($mref)); + $mref = $ibx->msg_by_smsg($cur) or return; + msg_str($ctx, Email::Simple->new($mref), $mref); } sub close {} # noop @@ -57,18 +58,17 @@ sub emit_raw { my ($ctx) = @_; my $mid = $ctx->{mid}; my $ibx = $ctx->{-inbox}; - my $first; - my $more; + my ($first, $mref, $more); if (my $over = $ibx->over) { my ($id, $prev); my $smsg = $over->next_by_mid($mid, \$id, \$prev) or return; - my $mref = $ibx->msg_by_smsg($smsg) or return; + $mref = $ibx->msg_by_smsg($smsg) or return; $first = Email::Simple->new($mref); my $next = $over->next_by_mid($mid, \$id, \$prev); # $more is for ->getline - $more = [ $ctx, $id, $prev, $next, $first ] if $next; + $more = [ $ctx, $id, $prev, $next, $first, $mref ] if $next; } else { - my $mref = $ibx->msg_by_mid($mid) or return; + $mref = $ibx->msg_by_mid($mid) or return; $first = Email::Simple->new($mref); } return unless defined $first; @@ -83,11 +83,12 @@ sub emit_raw { $fn .= '.txt'; } push @hdr, 'Content-Disposition', "inline; filename=$fn"; - [ 200, \@hdr, $more ? mb_stream($more) : [ msg_str($ctx, $first) ] ]; + [ 200, \@hdr, + $more ? mb_stream($more) : [ msg_str($ctx, $first, $mref) ] ]; } sub msg_str { - my ($ctx, $simple, $mid) = @_; # Email::Simple object + my ($ctx, $simple, $mref, $mid) = @_; # simple - Email::Simple object my $header_obj = $simple->header_obj; # drop potentially confusing headers, ssoma already should've dropped @@ -104,7 +105,7 @@ sub msg_str { 'List-Archive', "<$base>", 'List-Post', "{-primary_address}>", ); - my $crlf = $simple->crlf; + my $crlf = $header_obj->crlf; my $buf = "From mboxrd\@z Thu Jan 1 00:00:00 1970\n" . $header_obj->as_string; for (my $i = 0; $i < @append; $i += 2) { @@ -123,9 +124,8 @@ sub msg_str { # mboxrd quoting style # ref: http://www.qmail.org/man/man5/mbox.html - my $body = $simple->body; - $body =~ s/^(>*From )/>$1/gm; - $buf .= $body; + $$mref =~ s/^(>*From )/>$1/gm; + $buf .= $$mref; $buf .= "\n"; } @@ -268,9 +268,9 @@ sub getline { my ($self) = @_; my $ctx = $self->{ctx} or return; while (my $smsg = $self->{cb}->()) { - my $msg = $ctx->{-inbox}->msg_by_smsg($smsg) or next; - $msg = Email::Simple->new($msg); - $self->{gz}->write(PublicInbox::Mbox::msg_str($ctx, $msg, + my $mref = $ctx->{-inbox}->msg_by_smsg($smsg) or next; + my $s = Email::Simple->new($mref); + $self->{gz}->write(PublicInbox::Mbox::msg_str($ctx, $s, $mref, $smsg->{mid})); my $bref = $self->{buf}; if (length($$bref) >= 8192) { -- cgit v1.2.3-24-ge0c7 From 71ea9961786fa14ea0a67200847bac5a76abf751 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 27 Jun 2019 09:23:39 +0000 Subject: mbox: split header and body processing When dealing with ~30MB messages, we can save another ~30MB by splitting the header and body processing and not appending the body string back to the header. We'll rely on buffering in gzip or kernel (via MSG_MORE) to prevent silly packet sizes. --- lib/PublicInbox/Mbox.pm | 63 ++++++++++++++++++++++++++----------------------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/lib/PublicInbox/Mbox.pm b/lib/PublicInbox/Mbox.pm index 1bf71c60..0c3e52fe 100644 --- a/lib/PublicInbox/Mbox.pm +++ b/lib/PublicInbox/Mbox.pm @@ -16,8 +16,8 @@ use Email::Simple; use Email::MIME::Encode; sub subject_fn ($) { - my ($simple) = @_; - my $fn = $simple->header('Subject'); + my ($hdr) = @_; + my $fn = $hdr->header('Subject'); return 'no-subject' unless defined($fn); # no need for full Email::MIME, here @@ -36,20 +36,26 @@ sub mb_stream { } # called by PSGI server as body response +# this gets called twice for every message, once to return the header, +# once to retrieve the body sub getline { my ($more) = @_; # self - my ($ctx, $id, $prev, $next, $cur, $mref) = @$more; - if ($mref) { # first - pop @$more; - pop @$more; - return msg_str($ctx, $cur, $mref); + my ($ctx, $id, $prev, $next, $mref, $hdr) = @$more; + if ($hdr) { # first message hits this, only + pop @$more; # $hdr + return msg_hdr($ctx, $hdr); } - $cur = $next or return; + if ($mref) { # all messages hit this + pop @$more; # $mref + return msg_body($$mref); + } + my $cur = $next or return; my $ibx = $ctx->{-inbox}; $next = $ibx->over->next_by_mid($ctx->{mid}, \$id, \$prev); - @$more = ($ctx, $id, $prev, $next); # $next may be undef, here $mref = $ibx->msg_by_smsg($cur) or return; - msg_str($ctx, Email::Simple->new($mref), $mref); + $hdr = Email::Simple->new($mref)->header_obj; + @$more = ($ctx, $id, $prev, $next, $mref); # $next may be undef, here + msg_hdr($ctx, $hdr); # all but first message hits this } sub close {} # noop @@ -58,21 +64,17 @@ sub emit_raw { my ($ctx) = @_; my $mid = $ctx->{mid}; my $ibx = $ctx->{-inbox}; - my ($first, $mref, $more); + my ($mref, $more, $id, $prev, $next); if (my $over = $ibx->over) { - my ($id, $prev); my $smsg = $over->next_by_mid($mid, \$id, \$prev) or return; $mref = $ibx->msg_by_smsg($smsg) or return; - $first = Email::Simple->new($mref); - my $next = $over->next_by_mid($mid, \$id, \$prev); - # $more is for ->getline - $more = [ $ctx, $id, $prev, $next, $first, $mref ] if $next; + $next = $over->next_by_mid($mid, \$id, \$prev); } else { $mref = $ibx->msg_by_mid($mid) or return; - $first = Email::Simple->new($mref); } - return unless defined $first; - my $fn = subject_fn($first); + my $hdr = Email::Simple->new($mref)->header_obj; + $more = [ $ctx, $id, $prev, $next, $mref, $hdr ]; # for ->getline + my $fn = subject_fn($hdr); my @hdr = ('Content-Type'); if ($ibx->{obfuscate}) { # obfuscation is stupid, but maybe scrapers are, too... @@ -83,13 +85,11 @@ sub emit_raw { $fn .= '.txt'; } push @hdr, 'Content-Disposition', "inline; filename=$fn"; - [ 200, \@hdr, - $more ? mb_stream($more) : [ msg_str($ctx, $first, $mref) ] ]; + [ 200, \@hdr, mb_stream($more) ]; } -sub msg_str { - my ($ctx, $simple, $mref, $mid) = @_; # simple - Email::Simple object - my $header_obj = $simple->header_obj; +sub msg_hdr ($$;$) { + my ($ctx, $header_obj, $mid) = @_; # drop potentially confusing headers, ssoma already should've dropped # Lines and Content-Length @@ -121,12 +121,13 @@ sub msg_str { $buf .= "$k: $v$crlf" if defined $v; } $buf .= $crlf; +} +sub msg_body ($) { # mboxrd quoting style # ref: http://www.qmail.org/man/man5/mbox.html - $$mref =~ s/^(>*From )/>$1/gm; - $buf .= $$mref; - $buf .= "\n"; + $_[0] =~ s/^(>*From )/>$1/gm; + $_[0] .= "\n"; } sub thread_mbox { @@ -267,11 +268,13 @@ sub response { sub getline { my ($self) = @_; my $ctx = $self->{ctx} or return; + my $gz = $self->{gz}; while (my $smsg = $self->{cb}->()) { my $mref = $ctx->{-inbox}->msg_by_smsg($smsg) or next; - my $s = Email::Simple->new($mref); - $self->{gz}->write(PublicInbox::Mbox::msg_str($ctx, $s, $mref, - $smsg->{mid})); + my $h = Email::Simple->new($mref)->header_obj; + $gz->write(PublicInbox::Mbox::msg_hdr($ctx, $h, $smsg->{mid})); + $gz->write(PublicInbox::Mbox::msg_body($$mref)); + my $bref = $self->{buf}; if (length($$bref) >= 8192) { my $ret = $$bref; # copy :< -- cgit v1.2.3-24-ge0c7 From ea71a5606c633f82975e8208a6c552053f7f5af8 Mon Sep 17 00:00:00 2001 From: Eric Wong Date: Thu, 27 Jun 2019 17:31:30 +0000 Subject: nntp: reduce syscalls for ARTICLE and BODY Chances are we already have extra buffer space following the expensive LF => CRLF conversion that we can safely append an extra CRLF in those places without incurring a copy of the full string buffer. While we're at it, document where our pain points are in terms of memory usage, since tracking/controlling memory use isn't exactly obvious in high-level languages. Perhaps we should start storing messages in git as CRLF... --- lib/PublicInbox/NNTP.pm | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index 30d3dab6..5a886a3c 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -521,10 +521,12 @@ found: sub msg_body_write ($$) { my ($self, $msg) = @_; + + # these can momentarily double the memory consumption :< $$msg =~ s/^\./../smg; - $$msg =~ s/(?{article} = $art if defined $art && $art =~ /\A[0-9]+\z/; } -sub _header ($) { - my $hdr = $_[0]->as_string; +sub msg_hdr_write ($$$) { + my ($self, $hdr, $body_follows) = @_; + $hdr = $hdr->as_string; utf8::encode($hdr); - $hdr =~ s/(? article retrieved - head and body follow"); - msg_more($self, _header($hdr)); - msg_more($self, "\r\n"); + msg_hdr_write($self, $hdr, 1); msg_body_write($self, $msg); } @@ -566,7 +568,7 @@ sub cmd_head ($;$) { my ($n, $mid, undef, $hdr) = @$r; set_art($self, $art); more($self, "221 $n <$mid> article retrieved - head follows"); - msg_more($self, _header($hdr)); + msg_hdr_write($self, $hdr, 0); '.' } -- cgit v1.2.3-24-ge0c7