* [PATCH] speed.cgi: reduce system load by copying two general-functions.
@ 2021-10-22 8:05 Arne Fitzenreiter
2021-10-22 9:22 ` Bernhard Bitsch
2021-10-22 10:07 ` Michael Tremer
0 siblings, 2 replies; 5+ messages in thread
From: Arne Fitzenreiter @ 2021-10-22 8:05 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 3447 bytes --]
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] speed.cgi: reduce system load by copying two general-functions.
2021-10-22 8:05 [PATCH] speed.cgi: reduce system load by copying two general-functions Arne Fitzenreiter
@ 2021-10-22 9:22 ` Bernhard Bitsch
2021-10-22 10:07 ` Michael Tremer
1 sibling, 0 replies; 5+ messages in thread
From: Bernhard Bitsch @ 2021-10-22 9:22 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 3759 bytes --]
Reviewed-by: Bernhard Bitsch <bbitsch(a)ipfire.org>
Am 22.10.2021 um 10:05 schrieb Arne Fitzenreiter:
> 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;
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] speed.cgi: reduce system load by copying two general-functions.
2021-10-22 8:05 [PATCH] speed.cgi: reduce system load by copying two general-functions Arne Fitzenreiter
2021-10-22 9:22 ` Bernhard Bitsch
@ 2021-10-22 10:07 ` Michael Tremer
2021-10-22 10:30 ` Bernhard Bitsch
1 sibling, 1 reply; 5+ messages in thread
From: Michael Tremer @ 2021-10-22 10:07 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] speed.cgi: reduce system load by copying two general-functions.
2021-10-22 10:07 ` Michael Tremer
@ 2021-10-22 10:30 ` Bernhard Bitsch
2021-10-25 18:54 ` Michael Tremer
0 siblings, 1 reply; 5+ messages in thread
From: Bernhard Bitsch @ 2021-10-22 10:30 UTC (permalink / raw)
To: development
[-- Attachment #1: Type: text/plain, Size: 4966 bytes --]
Hi Michael,
you are right from a formal sight.
But testing the patch shows a reduction of system load in my system (
~50% ).
So the impact is not neglectable.
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.
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
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] speed.cgi: reduce system load by copying two general-functions.
2021-10-22 10:30 ` Bernhard Bitsch
@ 2021-10-25 18:54 ` Michael Tremer
0 siblings, 0 replies; 5+ messages in thread
From: Michael Tremer @ 2021-10-25 18:54 UTC (permalink / raw)
To: development
[-- 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
>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-10-25 18:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-22 8:05 [PATCH] speed.cgi: reduce system load by copying two general-functions Arne Fitzenreiter
2021-10-22 9:22 ` Bernhard Bitsch
2021-10-22 10:07 ` Michael Tremer
2021-10-22 10:30 ` Bernhard Bitsch
2021-10-25 18:54 ` Michael Tremer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox