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 DBD191F46C for ; Tue, 17 Sep 2019 08:31:23 +0000 (UTC) From: Eric Wong To: meta@public-inbox.org Subject: [PATCH 3/6] qspawn: shorten lifetime of circular references Date: Tue, 17 Sep 2019 08:31:20 +0000 Message-Id: <20190917083123.6468-4-e@80x24.org> In-Reply-To: <20190917083123.6468-1-e@80x24.org> References: <20190917083123.6468-1-e@80x24.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit List-Id: All of these circular references are designed to clear themselves, but these will make actual errors from Devel::Cycle easier-to-spot. The circular reference in the limiter {run_queue} is not a real problem, but we can avoid storing the circular reference until we actually need to spawn the child, reducing the size of the Qspawn object while it's in the queue, slightly. We also do not need to have redundant checks to spawn new processes, we should only spawn new processes when they're ->start-ed or after waitpid reaps them. --- lib/PublicInbox/Qspawn.pm | 28 +++++++++------------------- 1 file changed, 9 insertions(+), 19 deletions(-) diff --git a/lib/PublicInbox/Qspawn.pm b/lib/PublicInbox/Qspawn.pm index 844d50f..10fe534 100644 --- a/lib/PublicInbox/Qspawn.pm +++ b/lib/PublicInbox/Qspawn.pm @@ -44,11 +44,11 @@ sub new ($$$;) { } sub _do_spawn { - my ($self, $cb) = @_; + my ($self, $cb, $limiter) = @_; my $err; my ($cmd, $env, $opts) = @{$self->{args}}; my %opts = %{$opts || {}}; - my $limiter = $self->{limiter}; + $self->{limiter} = $limiter; foreach my $k (PublicInbox::Spawn::RLIMITS()) { if (defined(my $rlimit = $limiter->{$k})) { $opts{$k} = $rlimit; @@ -94,21 +94,21 @@ sub waitpid_err ($$) { $err = "W: waitpid($xpid, 0) => $pid: $!"; } # else should not be called with pid == 0 + my $env = delete $self->{env}; + # done, spawn whatever's in the queue my $limiter = $self->{limiter}; my $running = --$limiter->{running}; - # limiter->{max} may change dynamically - if (($running || $limiter->{running}) < $limiter->{max}) { - if (my $next = shift @{$limiter->{run_queue}}) { - _do_spawn(@$next); + if ($running < $limiter->{max}) { + if (my $next = shift(@{$limiter->{run_queue}})) { + _do_spawn(@$next, $limiter); } } return unless $err; $self->{err} = $err; - my $env = $self->{env} or return; - if (!$env->{'qspawn.quiet'}) { + if ($env && !$env->{'qspawn.quiet'}) { log_err($env, join(' ', @{$self->{args}}) . ": $err"); } } @@ -132,22 +132,12 @@ sub finish ($;$) { if (delete $self->{rpipe}) { do_waitpid($self, $env); } - - # limiter->{max} may change dynamically - my $limiter = $self->{limiter}; - if ($limiter->{running} < $limiter->{max}) { - if (my $next = shift @{$limiter->{run_queue}}) { - _do_spawn(@$next); - } - } } sub start { my ($self, $limiter, $cb) = @_; - $self->{limiter} = $limiter; - if ($limiter->{running} < $limiter->{max}) { - _do_spawn($self, $cb); + _do_spawn($self, $cb, $limiter); } else { push @{$limiter->{run_queue}}, [ $self, $cb ]; } -- EW