Ransom has asked for the wisdom of the Perl Monks concerning the following question:

This is more of a silly style question but the issue is cropping up more and more in the code I'm working on. Which of the following code block is more "correct"? Better yet, which provides better performance. 

foreach (@nodes)
{
    my $tdev = parse_node($_);
    push (@device, $tdev) if defined($tdev);
}

[download]

foreach (@nodes)
{
    push (@device, parse_node($_) if defined(parse_node($_);
}

[download]

My guess is that using the 2nd method would be faster, but only if parse_node is only called once for real. I'm afraid that the sub would get called twice and thus be twice as slow. On the other hand, creating a temp variable for each entry must have some overhead to worry about. In reality it doesn't matter for this project, but I'd like to know before I make this mistake in a bad place.

Replies are listed 'Best First'.
Re: Temp variable performace vs Inline behavior
by BrowserUk (Patriarch) on Jun 27, 2012 at 15:05 UTC

    Apart from the fact that, as posted, your two snippets do different things (and the second is missing a ')'), I'd do that this way:

    my @devices = grep{ defined( $_ = parse_node( $_ ) ) } @nodes;

    [download]
    With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
    Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
    "Science is about questioning the status quo. Questioning authority".
    In the absence of evidence, opinion is indistinguishable from prejudice.

    The start of some sanity?
[reply]
[d/l]

      BrowserUk’s solution:

      my @devices = grep{ defined( $_ = parse_node( $_ ) ) } @nodes;

      [download]

      is compact and efficient, but has one side-effect: the assignment to $_ clobbers the contents of @nodes.

      In cases where this is undesirable, the following seems to do the same without the side-effect:

      my @devices = grep { defined } map { parse_node( $_ ) } @nodes;

      [download]

      Or is there a simpler way?

      Athanasius <°(((>< contra mundum

[reply]
[d/l]
[select]

        You could use the same technique but clobber a temporary array instead...

        my @devices =
  grep { defined($_ = parse_node($_)) }
  my @tmp = @nodes;

        [download]

        Or you could use map:

        my @devices =
  map { my $x; defined($x=parse_node($_)) ? $x : () }
  @nodes;

        [download]

        If parse_node was written to return the empty list in cases of failure (rather than returning the scalar undef), then it would just be:

        my @devices = map { parse_node($_) } @nodes;

        [download]
        perl -E'sub Monkey::do{say$_,for@_,do{($monkey=[caller(0)]->[3])=~s{::}{ }and$monkey}}"Monkey say"->Monkey::do'
[reply]
[d/l]
[select]
      Whoops about the typo. I don't understand how they are different?

      I've tested my script with both and get the same results. parse_node return a nodelet on success or undef on failure. Where is the difference?

[reply]

        Calling parse_node() twice in order to avoid a temporary variable, is very unlikely to effect a performance gain.

        Unless parse_node() does almost nothing, in which case you would certainly gain more by in-lining it code in the loop. The grep solution will almost certainly prove faster.

        Either way, it probably isn't worth the effort unless this is likely to be called thousands or millions of times.

        My philosophy is "Optimise if it is worth it!". And that means concentrating ones effort where gains are likely to be measurable -- in the overall runtime.

        With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
        Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
        "Science is about questioning the status quo. Questioning authority".
        In the absence of evidence, opinion is indistinguishable from prejudice.

        The start of some sanity?
[reply]
[d/l]
[select]
        Where is the difference?

        In the number of times parse_node is called, for starters.

[reply]
[d/l]
Re: Temp variable performace vs Inline behavior
by muba (Priest) on Jun 27, 2012 at 15:10 UTC

    I'd go for the first version, with the temporary variable, for the exact reason you mention: the second version has an extra call to the subroutine. The extra memory overhead shouldn't be much of a problem. I don't think you're dealing with gigabites of data here at once.

    Another reason the first version is preferable because the call to the subroutine in the if clause may have side effects that will affect the value that will be pushed onto the array. The following script demonstrates what I mean.

    Update: there was a stupid little error in the script. Fixed.

    use strict;
use warnings;
use subs qw(
    version1
    version2
);

my @delete = qw(bar baz);
my %hash = (foo => 1, bar => 2, baz => 3);
print "Keys in the hash: ", join(", ", keys %hash), "\n";

print "Version 1 reports ", version1, " as the sum of values deleted f
+rom the hash.\n";
print "Keys in the hash after version 1: ", join(", ", keys %hash), "\
+n";

print "\n";

%hash = (foo => 1, bar => 2, baz => 3);
print "Keys in the hash: ", join(", ", keys %hash), "\n";
print "Version 2 reports ", version2, " as the sum of values deleted f
+rom the hash.\n";
print "Keys in the hash after version 2: ", join(", ", keys %hash), "\
+n";


sub version1 {
    my $num_deleted = 0;
    for (@delete) {
        my $deleted = delete $hash{$_};
        $num_deleted += $deleted if $deleted;
    }
    return $num_deleted
}

sub version2 {
    my $num_deleted = 0;
    for (@delete) {
        $num_deleted += delete $hash{$_} if delete $hash{$_}
    }
    return $num_deleted
}

__END__
Keys in the hash: bar, baz, foo
Version 1 reports 5 as the sum of values deleted from the has.
Keys in the hash after version 1: foo

Keys in the hash: bar, baz, foo
Use of uninitialized value in addition (+) at G:\y.pl line 35.
Use of uninitialized value in addition (+) at G:\y.pl line 35.
Version 2 reports 0 as the sum of values deleted from the hash.
Keys in the hash after version 2: foo

    [download]
[reply]
[d/l]
      Didn't even think about side effects!

      Thanks for the example and snippet. It's good to know that parse_node does indeed get called twice.

[reply]
Re: Temp variable performace vs Inline behavior
by GrandFather (Saint) on Jun 28, 2012 at 01:15 UTC
    creating a temp variable for each entry must have some overhead to worry about

    Umm, no, actually creating a temporary variable almost never has enough overhead to worry about. Creating a temporary copy of a large array or hash may impose enough overhead to worry about, but you'd find that out when you find that there is a speed (or other) issue and profile the code. Don't premature optimise code. Write code so it is clear, correct and maintainable first, then optimise if there is a problem.

    True laziness is hard work
[reply]
Re: Temp variable performace vs Inline behavior
by frozenwithjoy (Priest) on Jun 27, 2012 at 15:12 UTC
    Of those two, I'd choose the first so you don't call parse_node() twice. Alternatively, you could change the subroutine to take your entire array and do the checks within the subroutine. Then you could just write my @device = parse_node(@nodes).
[reply]
[d/l]
[select]
Re: Temp variable performace vs Inline behavior
by bulk88 (Priest) on Jun 28, 2012 at 01:30 UTC 
    use strict;
use Data::Dumper;
use Devel::Peek;
use B::Deparse;
use B::Concise;
use Devel::Size qw(size total_size);

sub big {
    my (@nodes, @device);    
    foreach (@nodes) {
        my $tdev = parse_node($_);
        push (@device, $tdev) if defined($tdev);
    }
}

sub medium {
    my (@nodes, @device);
    foreach (@nodes) {
        push (@device, parse_node($_)) if defined(parse_node($_));
    }
}

sub small {
    my (@nodes, @device, $result);
    foreach (@nodes) {
        push (@device, $result) if defined($result = parse_node($_)); 
    }
}

sub smallx {
    my (@nodes, @device, $result);
    foreach  (@nodes) {
        defined($result = parse_node($_)) ? push (@device, $result): 0
+;
    }
}
sub notsmall {
    my (@nodes, @device);
    foreach  (@nodes) {
        push (@device, $_) if defined($_ = parse_node($_)); 
    }
}
foreach (\&big, \&medium, \&small, \&smallx, \&notsmall) {DumpStats($_
+);}
sub DumpStats {
my $deparse = B::Deparse->new();
print $deparse->coderef2text($_[0]);
print "\n\n".size($_[0])."\n\n";
my $walker = B::Concise::compile('-src','-exec',$_[0]);
$walker->();
}

    [download]
    small and smallx win 
    {
    use strict 'refs';
    my(@nodes, @device);
    foreach $_ (@nodes) {
        my $tdev = parse_node($_);
        push @device, $tdev if defined $tdev;
    }
}

1349

B::Concise::compile(CODE(0x829fac))
# 9:     my (@nodes, @device);    
1  <;> nextstate(main -4 n13.pl:9) v:*,&,$
2  <0> pushmark vM/128
3  <0> padav[@nodes:-4,0] vM/LVINTRO
4  <0> padav[@device:-4,0] vM/LVINTRO
5  <@> list vKPM/128
# 10:     foreach (@nodes) {
6  <;> nextstate(main -1 n13.pl:10) v:*,&,{,$
7  <0> pushmark sM
8  <0> padav[@nodes:-4,0] sRM
9  <#> gv[*_] s
a  <{> enteriter(next->q last->t redo->b) lKS/8
r  <0> iter s
s  <|> and(other->b) K/1
# 11:         my $tdev = parse_node($_);
b      <;> nextstate(main -3 n13.pl:11) v:*,&,{,$
c      <0> pushmark s
d      <#> gvsv[*_] s
e      <#> gv[*parse_node] s/EARLYCV
f      <1> entersub[t7] sKS/TARG,3
g      <0> padsv[$tdev:-3,-2] sRM*/LVINTRO
h      <2> sassign vKS/2
# 12:         push (@device, $tdev) if defined($tdev);
i      <;> nextstate(main -2 n13.pl:12) v:*,&,{,$
j      <0> padsv[$tdev:-3,-2] s
k      <1> defined sK/1
l      <|> and(other->m) vK/1
m          <0> pushmark s
n          <0> padav[@device:-4,0] lRM
o          <0> padsv[$tdev:-3,-2] l
p          <@> push[t8] vK/2
q      <0> unstack s
           goto r
t  <2> leaveloop K/2
u  <1> leavesub[1 ref] K/REFC,1
{
    use strict 'refs';
    my(@nodes, @device);
    foreach $_ (@nodes) {
        push @device, parse_node($_) if defined parse_node($_);
    }
}

1320

B::Concise::compile(CODE(0xc3020c))
# 17:     my (@nodes, @device);
v  <;> nextstate(main 1 n13.pl:17) v:*,&,$
w  <0> pushmark vM/128
x  <0> padav[@nodes:1,4] vM/LVINTRO
y  <0> padav[@device:1,4] vM/LVINTRO
z  <@> list vKPM/128
# 18:     foreach (@nodes) {
10 <;> nextstate(main 3 n13.pl:18) v:*,&,{,$
11 <0> pushmark sM
12 <0> padav[@nodes:1,4] sRM
13 <#> gv[*_] s
14 <{> enteriter(next->1j last->1m redo->15) lKS/8
1k <0> iter s
1l <|> and(other->15) K/1
# 19:         push (@device, parse_node($_)) if defined(parse_node($_)
+);
15     <;> nextstate(main 2 n13.pl:19) v:*,&,{,$
16     <0> pushmark s
17     <#> gvsv[*_] s
18     <#> gv[*parse_node] s/EARLYCV
19     <1> entersub[t11] sKS/TARG,3
1a     <1> defined sK/1
1b     <|> and(other->1c) vK/1
1c         <0> pushmark s
1d         <0> padav[@device:1,4] lRM
1e         <0> pushmark s
1f         <#> gvsv[*_] s
1g         <#> gv[*parse_node] s/EARLYCV
1h         <1> entersub[t6] lKS/TARG,3
1i         <@> push[t7] vK/2
1j     <0> unstack s
           goto 1k
1m <2> leaveloop K/2
1n <1> leavesub[1 ref] K/REFC,1
{
    use strict 'refs';
    my(@nodes, @device, $result);
    foreach $_ (@nodes) {
        push @device, $result if defined($result = parse_node($_));
    }
}

1291

B::Concise::compile(CODE(0xbbdd24))
# 24:     my (@nodes, @device, $result);
1o <;> nextstate(main 5 n13.pl:24) v:*,&,$
1p <0> pushmark vM/128
1q <0> padav[@nodes:5,8] vM/LVINTRO
1r <0> padav[@device:5,8] vM/LVINTRO
1s <0> padsv[$result:5,8] vM/LVINTRO
1t <@> list vKPM/128
# 25:     foreach (@nodes) {
1u <;> nextstate(main 7 n13.pl:25) v:*,&,{,$
1v <0> pushmark sM
1w <0> padav[@nodes:5,8] sRM
1x <#> gv[*_] s
1y <{> enteriter(next->2c last->2f redo->1z) lKS/8
2d <0> iter s
2e <|> and(other->1z) K/1
# 26:         push (@device, $result) if defined($result = parse_node(
+$_)); 
1z     <;> nextstate(main 6 n13.pl:26) v:*,&,{,$
20     <0> pushmark s
21     <#> gvsv[*_] s
22     <#> gv[*parse_node] s/EARLYCV
23     <1> entersub[t8] sKS/TARG,3
24     <0> padsv[$result:5,8] sRM*
25     <2> sassign sKS/2
26     <1> defined sK/1
27     <|> and(other->28) vK/1
28         <0> pushmark s
29         <0> padav[@device:5,8] lRM
2a         <0> padsv[$result:5,8] l
2b         <@> push[t4] vK/2
2c     <0> unstack s
           goto 2d
2f <2> leaveloop K/2
2g <1> leavesub[1 ref] K/REFC,1
{
    use strict 'refs';
    my(@nodes, @device, $result);
    foreach $_ (@nodes) {
        defined($result = parse_node($_)) ? push(@device, $result) : '
+???';
    }
}

1292

B::Concise::compile(CODE(0xc00454))
# 31:     my (@nodes, @device, $result);
2h <;> nextstate(main 9 n13.pl:31) v:*,&,$
2i <0> pushmark vM/128
2j <0> padav[@nodes:9,12] vM/LVINTRO
2k <0> padav[@device:9,12] vM/LVINTRO
2l <0> padsv[$result:9,12] vM/LVINTRO
2m <@> list vKPM/128
# 32:     foreach  (@nodes) {
2n <;> nextstate(main 11 n13.pl:32) v:*,&,{,$
2o <0> pushmark sM
2p <0> padav[@nodes:9,12] sRM
2q <#> gv[*_] s
2r <{> enteriter(next->35 last->38 redo->2s) lKS/8
36 <0> iter s
37 <|> and(other->2s) K/1
# 33:         defined($result = parse_node($_)) ? push (@device, $resu
+lt): 0;
2s     <;> nextstate(main 10 n13.pl:33) v:*,&,{,$
2t     <0> pushmark s
2u     <#> gvsv[*_] s
2v     <#> gv[*parse_node] s/EARLYCV
2w     <1> entersub[t7] sKS/TARG,3
2x     <0> padsv[$result:9,12] sRM*
2y     <2> sassign sKS/2
2z     <1> defined sK/1
30     <|> cond_expr(other->31) vK/1
31         <0> pushmark s
32         <0> padav[@device:9,12] lRM
33         <0> padsv[$result:9,12] l
34         <@> push[t8] vK/2
35     <0> unstack s
           goto 36
38 <2> leaveloop K/2
39 <1> leavesub[1 ref] K/REFC,1
{
    use strict 'refs';
    my(@nodes, @device);
    foreach $_ (@nodes) {
        push @device, $_ if defined($_ = parse_node($_));
    }
}

1330

B::Concise::compile(CODE(0xc00324))
# 37:     my (@nodes, @device);
3a <;> nextstate(main 13 n13.pl:37) v:*,&,$
3b <0> pushmark vM/128
3c <0> padav[@nodes:13,16] vM/LVINTRO
3d <0> padav[@device:13,16] vM/LVINTRO
3e <@> list vKPM/128
# 38:     foreach  (@nodes) {
3f <;> nextstate(main 15 n13.pl:38) v:*,&,{,$
3g <0> pushmark sM
3h <0> padav[@nodes:13,16] sRM
3i <#> gv[*_] s
3j <{> enteriter(next->3x last->40 redo->3k) lKS/8
3y <0> iter s
3z <|> and(other->3k) K/1
# 39:         push (@device, $_) if defined($_ = parse_node($_)); 
3k     <;> nextstate(main 14 n13.pl:39) v:*,&,{,$
3l     <0> pushmark s
3m     <#> gvsv[*_] s
3n     <#> gv[*parse_node] s/EARLYCV
3o     <1> entersub[t9] sKS/TARG,3
3p     <#> gvsv[*_] s
3q     <2> sassign sKS/2
3r     <1> defined sK/1
3s     <|> and(other->3t) vK/1
3t         <0> pushmark s
3u         <0> padav[@device:13,16] lRM
3v         <#> gvsv[*_] s
3w         <@> push[t4] vK/2
3x     <0> unstack s
           goto 3y
40 <2> leaveloop K/2
41 <1> leavesub[1 ref] K/REFC,1

    [download]
    My Devel::Size is not the stock CPAN Devel::Size, so my numbers are much lower than the CPAN version but relative to each other they should be fine. Getting rid of the nextstate op for the block of the foreach loop interestingly did not decrease the size the ::Size reported, IDK why. I know that the nextstate ops do execute if I step through runops in C.
[reply]
[d/l]
[select]
Re: Temp variable performace vs Inline behavior
by locked_user sundialsvc4 (Abbot) on Jun 27, 2012 at 22:31 UTC

    Apart from the performance/capacity “edge cases” that I openly acknowledge BrowserUK (in particular) deals with every day, my bright-line rule always tilts in favor of: clarity, and maintainability.   In the world that I live in every day, it is “six o’one, half a dozen of the other” in terms of performance ... because raw speed in my world is rarely actually the issue.   (And to the extent that it may be, I look for a deeper algorithm change.)  

    Certainly, to me, the first example is clear, and the second example would instantly fail my code-review:   in the second case, the code does not plainly say that parse_node() will be called once vs. twice.   I emphatically do not want to have to “understand Perl” in order to understand what a line of source-code says.   I want it to very plainly and unambiguously say precisely what it means.   (And if the compiler’s optimizer can improve upon it, goody for the optimizer and thank you for the freebie.)

    Spoken from the point-of-view of a businessman who did lose $10,000.00(USD) in actual cash, that he most-emphatically could not afford to lose but also could not rightfully keep, due to code written by someone else that was not clear.   The cause of project-death was “cleverness.”   I could not find the bug because I could not identify it.   I had to borrow money fast.

      I emphatically do not want to have to “understand Perl”

      Hoisted by your own petard!

      And it probably explains:

      I could not find the bug because I could not identify it.

      Would you trust or employ a lowyer who spoke English, but wasn't familiar with legal terminology?

      For example: One who didn't know the difference between "chattels" and "property"?

      Why should anyone employ a programmer for a Perl project, that knew COBOL, but didn't: ' “understand Perl” '?

      With the rise and rise of 'Social' network sites: 'Computers are making people easier to use everyday'
      Examine what is said, not who speaks -- Silence betokens consent -- Love the truth but pardon error.
      "Science is about questioning the status quo. Questioning authority".
      In the absence of evidence, opinion is indistinguishable from prejudice.

      The start of some sanity?
[reply]

Back to Seekers of Perl Wisdom