Re: [PATCH] speed.cgi: reduce system load by copying two general-functions.

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 5603 bytes --]

Hello,

> On 22 Oct 2021, at 11:30, Bernhard Bitsch <bbitsch(a)ipfire.org> wrote:
> 
> Hi Michael,
> 
> you are right from a formal sight.
> But testing the patch shows a reduction of system load in my system ( ~50% ).

This is insane that loading a Perl module takes that many resources.

> So the impact is not neglectable.

I didn’t dispute that :)

> Because we should not copy functions, as stated, we must review these Perl modules. This will be an ongoing process. I fear, we will find some copys scattered in various files.
> One thing I noticed while reading the various sources, is the relation of general-functions.pl and network-functions.pl. general-functions.pl includes the latter ( everytime ) and contains also network related functions. Maybe this is a start point for brushing up.

Yes, generally it would be good to have functions where they should be. Unfortunately there are lots of “wrong” implementations that behave a certain way and the code that calls them does unexpected stuff when a function returns something else.

This is a large mess and I would like to avoid making it any worse.

-Michael

> 
> Regards,
> Bernhard
> 
> 
> Am 22.10.2021 um 12:07 schrieb Michael Tremer:
>> Hello Arne,
>> I do not think I am happy with this patch because it duplicates code.
>> Whenever we change or fix anything in any of those copied functions, we *will* forget the copy in speed.cgi.
>> What is the reason we cannot load general-funcions.pl in speed.cgi?
>> Loading a slightly larger Perl file like general-functions.pl cannot impact the performance of the interpreter. If it does, the way should be to fix any slow parts in it.
>> -Michael
>>> On 22 Oct 2021, at 09:05, Arne Fitzenreiter <arne_f(a)ipfire.org> wrote:
>>> 
>>> include general-functions.pl load and initialize many subfunctions that are not
>>> needed by speed.cgi which was executed very ofthen.
>>> So this reduce the system load significant if webif was open in browser
>>> and ajax-speed display enabled.
>>> 
>>> Signed-off-by: Arne Fitzenreiter <arne_f(a)ipfire.org>
>>> ---
>>> config/cfgroot/general-functions.pl |  2 ++
>>> html/cgi-bin/speed.cgi              | 46 +++++++++++++++++++++++++++--
>>> 2 files changed, 45 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/config/cfgroot/general-functions.pl b/config/cfgroot/general-functions.pl
>>> index f72d6588c..c10a04faa 100644
>>> --- a/config/cfgroot/general-functions.pl
>>> +++ b/config/cfgroot/general-functions.pl
>>> @@ -53,6 +53,7 @@ sub system_background($) {
>>> }
>>> 
>>> # Returns the output of a shell command
>>> +# if you change this also check speed.cgi that include a local copy for systemload reasons
>>> sub system_output($) {
>>> 	my @command = @_;
>>> 	my $pid;
>>> @@ -1227,6 +1228,7 @@ sub firewall_reload() {
>>> }
>>> 
>>> # Function which will return the used interface for the red network zone (red0, ppp0, etc).
>>> +# if you change this also check speed.cgi that include a local copy for systemload reasons
>>> sub get_red_interface() {
>>> 
>>> 	open(IFACE, "${General::swroot}/red/iface") or die "Could not open /var/ipfire/red/iface";
>>> diff --git a/html/cgi-bin/speed.cgi b/html/cgi-bin/speed.cgi
>>> index b550fda52..651c3c0b3 100644
>>> --- a/html/cgi-bin/speed.cgi
>>> +++ b/html/cgi-bin/speed.cgi
>>> @@ -19,7 +19,47 @@
>>> #                                                                             #
>>> ###############################################################################
>>> 
>>> -require '/var/ipfire/general-functions.pl';
>>> +###############################################################################
>>> +# functions copied from general-functions.pl for speed improvement because    #
>>> +# loading and initializing the whole general-functions.pl every second create #
>>> +# high system load                                                            #
>>> +###########################################OA##################################
>>> +#
>>> +# Returns the output of a shell command
>>> +sub General__system_output($) {
>>> +    my @command = @_;
>>> +    my $pid;
>>> +    my @output = ();
>>> +
>>> +    unless ($pid = open(OUTPUT, "-|")) {
>>> +            open(STDERR, ">&STDOUT");
>>> +            exec { ${command[0]} } @command;
>>> +            die "Could not execute @command: $!";
>>> +    }
>>> +
>>> +    waitpid($pid, 0);
>>> +
>>> +    while (<OUTPUT>) {
>>> +            push(@output, $_);
>>> +    }
>>> +
>>> +    close(OUTPUT);
>>> +    return @output;
>>> +}
>>> +#
>>> +# Function which will return the used interface for the red network zone (red0, ppp0, etc).
>>> +sub General__get_red_interface() {
>>> +
>>> +     open(IFACE, "/var/ipfire/red/iface") or die "Could not open /var/ipfire/red/iface";
>>> +
>>> +     my $interface = <IFACE>;
>>> +     close(IFACE);
>>> +     chomp $interface;
>>> +
>>> +     return $interface;
>>> +}
>>> +#
>>> +###############################################################################
>>> 
>>> my $data_last = $ENV{'QUERY_STRING'};
>>> my $rxb_last = 0;
>>> @@ -38,8 +78,8 @@ foreach $field (@fields) {
>>> 	}
>>> }
>>> 
>>> -my $interface = &General::get_red_interface();
>>> -my @data_now = &General::system_output("ip", "-s", "link", "show", "$interface");
>>> +my $interface = &General__get_red_interface();
>>> +my @data_now = &General__system_output("ip", "-s", "link", "show", "$interface");
>>> 
>>> my $lastline;
>>> my $rxb_now = 0;
>>> -- 
>>> 2.25.1
>>>