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,AWL,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 8D1CF1F609 for ; Mon, 10 Jun 2019 05:18:46 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 1/9] ds: simplify write buffer accounting Date: Mon, 10 Jun 2019 05:18:38 +0000 Message-Id: <20190610051846.26757-2-e@80x24.org> In-Reply-To: <20190610051846.26757-1-e@80x24.org> References: <20190610051846.26757-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: Keeping track of write_buf_size was redundant and pointless when we can simply check the number of elements in the buffer array. Multiple sources of truth leads to confusion; confusion leads to bugs. Finally, rename the prefixes to 'wbuf' to ensure we loudly (instead of silently) break any external dependencies being ported over from Danga::Socket, as further changes are pending. --- lib/PublicInbox/DS.pm | 41 ++++++++++++++-------------------- lib/PublicInbox/HTTP.pm | 6 ++--- lib/PublicInbox/HTTPD/Async.pm | 2 +- lib/PublicInbox/NNTP.pm | 8 +++---- 4 files changed, 25 insertions(+), 32 deletions(-) diff --git a/lib/PublicInbox/DS.pm b/lib/PublicInbox/DS.pm index 03612ce8..172a9f52 100644 --- a/lib/PublicInbox/DS.pm +++ b/lib/PublicInbox/DS.pm @@ -27,9 +27,8 @@ use PublicInbox::Syscall qw(:epoll); use fields ('sock', # underlying socket 'fd', # numeric file descriptor - 'write_buf', # arrayref of scalars, scalarrefs, or coderefs to write - 'write_buf_offset', # offset into first array of write_buf to start writing at - 'write_buf_size', # total length of data in all write_buf items + 'wbuf', # arrayref of scalars, scalarrefs, or coderefs to write + 'wbuf_off', # offset into first element of wbuf to start writing at 'closed', # bool: socket is closed 'event_watch', # bitmask of events the client is interested in (POLLIN,OUT,etc.) ); @@ -449,9 +448,8 @@ sub new { unless $sock && $fd; $self->{fd} = $fd; - $self->{write_buf} = []; - $self->{write_buf_offset} = 0; - $self->{write_buf_size} = 0; + $self->{wbuf} = []; + $self->{wbuf_off} = 0; $self->{closed} = 0; my $ev = $self->{event_watch} = POLLERR|POLLHUP|POLLNVAL; @@ -552,7 +550,7 @@ sub _cleanup { # we need to flush our write buffer, as there may # be self-referential closures (sub { $client->close }) # preventing the object from being destroyed - $self->{write_buf} = []; + @{$self->{wbuf}} = (); # if we're using epoll, we have to remove this from our epoll fd so we stop getting # notifications about it @@ -612,12 +610,12 @@ sub write { # just queue data if there's already a wait my $need_queue; + my $wbuf = $self->{wbuf}; if (defined $data) { $bref = ref $data ? $data : \$data; - if ($self->{write_buf_size}) { - push @{$self->{write_buf}}, $bref; - $self->{write_buf_size} += ref $bref eq "SCALAR" ? length($$bref) : 1; + if (scalar @$wbuf) { + push @$wbuf, $bref; return 0; } @@ -629,7 +627,7 @@ sub write { WRITE: while (1) { - return 1 unless $bref ||= $self->{write_buf}[0]; + return 1 unless $bref ||= $wbuf->[0]; my $len; eval { @@ -638,8 +636,7 @@ sub write { if ($@) { if (UNIVERSAL::isa($bref, "CODE")) { unless ($need_queue) { - $self->{write_buf_size}--; # code refs are worth 1 - shift @{$self->{write_buf}}; + shift @$wbuf; } $bref->(); @@ -654,9 +651,9 @@ sub write { die "Write error: $@ <$bref>"; } - my $to_write = $len - $self->{write_buf_offset}; + my $to_write = $len - $self->{wbuf_off}; my $written = syswrite($self->{sock}, $$bref, $to_write, - $self->{write_buf_offset}); + $self->{wbuf_off}); if (! defined $written) { if ($! == EPIPE) { @@ -665,8 +662,7 @@ sub write { # since connection has stuff to write, it should now be # interested in pending writes: if ($need_queue) { - push @{$self->{write_buf}}, $bref; - $self->{write_buf_size} += $len; + push @$wbuf, $bref; } $self->watch_write(1); return 0; @@ -681,19 +677,17 @@ sub write { DebugLevel >= 2 && $self->debugmsg("Wrote PARTIAL %d bytes to %d", $written, $self->{fd}); if ($need_queue) { - push @{$self->{write_buf}}, $bref; - $self->{write_buf_size} += $len; + push @$wbuf, $bref; } # since connection has stuff to write, it should now be # interested in pending writes: - $self->{write_buf_offset} += $written; - $self->{write_buf_size} -= $written; + $self->{wbuf_off} += $written; $self->on_incomplete_write; return 0; } elsif ($written == $to_write) { DebugLevel >= 2 && $self->debugmsg("Wrote ALL %d bytes to %d (nq=%d)", $written, $self->{fd}, $need_queue); - $self->{write_buf_offset} = 0; + $self->{wbuf_off} = 0; $self->watch_write(0); # this was our only write, so we can return immediately @@ -702,8 +696,7 @@ sub write { # can't be anything else to write. return 1 if $need_queue; - $self->{write_buf_size} -= $written; - shift @{$self->{write_buf}}; + shift @$wbuf; undef $bref; next WRITE; } diff --git a/lib/PublicInbox/HTTP.pm b/lib/PublicInbox/HTTP.pm index 977614b4..fd103251 100644 --- a/lib/PublicInbox/HTTP.pm +++ b/lib/PublicInbox/HTTP.pm @@ -260,7 +260,7 @@ sub getline_cb ($$$) { $write->($buf); # may close in PublicInbox::DS::write unless ($self->{closed}) { my $next = $self->{pull}; - if ($self->{write_buf_size}) { + if (scalar @{$self->{wbuf}}) { $self->write($next); } else { PublicInbox::EvCleanup::asap($next); @@ -315,7 +315,7 @@ use constant MSG_MORE => ($^O eq 'linux') ? 0x8000 : 0; sub more ($$) { my $self = $_[0]; return if $self->{closed}; - if (MSG_MORE && !$self->{write_buf_size}) { + if (MSG_MORE && !scalar(@{$self->{wbuf}})) { my $n = send($self->{sock}, $_[1], MSG_MORE); if (defined $n) { my $nlen = length($_[1]) - $n; @@ -487,7 +487,7 @@ sub close { # for graceful shutdown in PublicInbox::Daemon: sub busy () { my ($self) = @_; - ($self->{rbuf} ne '' || $self->{env} || $self->{write_buf_size}); + ($self->{rbuf} ne '' || $self->{env} || scalar(@{$self->{wbuf}})); } 1; diff --git a/lib/PublicInbox/HTTPD/Async.pm b/lib/PublicInbox/HTTPD/Async.pm index dbe8a84a..60701085 100644 --- a/lib/PublicInbox/HTTPD/Async.pm +++ b/lib/PublicInbox/HTTPD/Async.pm @@ -46,7 +46,7 @@ sub main_cb ($$$) { if ($r) { $fh->write($$bref); unless ($http->{closed}) { # PublicInbox::DS sets this - if ($http->{write_buf_size}) { + if (scalar @{$http->{wbuf}}) { $self->watch_read(0); $http->write(restart_read_cb($self)); } diff --git a/lib/PublicInbox/NNTP.pm b/lib/PublicInbox/NNTP.pm index be80560f..b62c2187 100644 --- a/lib/PublicInbox/NNTP.pm +++ b/lib/PublicInbox/NNTP.pm @@ -619,7 +619,7 @@ sub long_response ($$) { update_idle_time($self); check_read($self); } - } elsif ($more) { # $self->{write_buf_size}: + } elsif ($more) { # scalar @{$self->{wbuf}}: # no recursion, schedule another call ASAP # but only after all pending writes are done update_idle_time($self); @@ -925,7 +925,7 @@ use constant MSG_MORE => ($^O eq 'linux') ? 0x8000 : 0; sub do_more ($$) { my ($self, $data) = @_; - if (MSG_MORE && !$self->{write_buf_size}) { + if (MSG_MORE && !scalar(@{$self->{wbuf}})) { my $n = send($self->{sock}, $data, MSG_MORE); if (defined $n) { my $dlen = length($data); @@ -1004,8 +1004,8 @@ sub not_idle_long ($$) { # for graceful shutdown in PublicInbox::Daemon: sub busy { my ($self, $now) = @_; - ($self->{rbuf} ne '' || $self->{long_res} || $self->{write_buf_size} || - not_idle_long($self, $now)); + ($self->{rbuf} ne '' || $self->{long_res} || + scalar(@{$self->{wbuf}}) || not_idle_long($self, $now)); } 1; -- EW