in reply to Re^4: Some code optimization
in thread Some code optimization

I think the problem starts with the first problematic function...

Now you aren't making sense. According to your initial description of timing relations:

65 sec for full code 15 sec for code MINUS "is_contained()" call (2nd "next" uncommented)
That really makes it look like "is_contained()" accounts for most of the run time, which I would expect, since that is the part that involves several more layers of function calls, with lots of min, max, sort, grep, etc going on (some of it obviously not needed). "Look into it", for sure.

To reiterate the question in my update: is there a way to move the sorting out of the innermost loop? E.g. might there be an appropriate "sort" on one or both of the lists that drive the two "foreach" loops in "scenario()", which would simplify operations inside the inner loop? Can anything be done as part of the outer foreach so that you don't have to do it repeatedly as part of the inner loop?

Replies are listed 'Best First'.
Re^6: Some code optimization
by roibrodo (Sexton) on Jun 18, 2010 at 10:03 UTC

    "is_contained()" indeed accounts for most of the run time, but "gene_to_legal_range()" is also much slower than I would expect.

    Updated benchmarks (tested again with the original posted code):
    - Both subroutines executed: ~64 seconds
    - Only "gene_to_legal_range()" is executed: ~15 seconds
    - Only "gene_to_legal_range()" is executed but its body replaced with just a return: ~6.1 seconds
    - none is executed: ~4.5 seconds

    So "gene_to_legal_range()", which is quite simple, triples the run time (15 vs 4.5). Also just calling this subroutine, even if it does nothing (just a return) adds some significant time (6.1 vs 4.5).

    In the final program I will have ~1000 iterations (instead of 50) so these differences do matter.

    To conclude, I agree that "is_contained()" consumes more time, but I suggest that we first assert that the first, much simpler function is "optimal" Since I'm new to perl optimization, this might also help me know what to expect, what is considered OK (e.g. such an overhead for a subroutine call?) and what is not.

    UPDATE: and as for sort, again, I removed it completely. Got ~60 seconds (instead of ~64). But I again suggest we first "clear" with the first, simpler sub.

      To conclude, I agree that "is_contained()" consumes more time, but I suggest that we first assert that the first, much simpler function is "optimal"

      Suit yourself, but to me your statement is a clear example of misplaced priorities, bordering on irrational obsession. The few changes I suggested save an amount of time equal to the total consumed by that first function.

      Since I'm new to perl optimization...

      ... or to optimization in general? If you want to focus on optimizing that first function, you should be coding it in C (and that'll be a waste of time if you don't fix the second function, and/or move stuff from inner to outer loops, or just work out a better approach from first principles).

      UPDATE: BTW, did you happen to try moving the logic of that first function into the caller? (ie. do that stuff in-line in "scenario()" rather than as a sub call to "gene_to_legal_range") -- that might trim your 16 sec. test case down to 13 (about 20%, which is respectable).

        My goal is to optimize both. Since I'm new to this aspect of perl I simply suggested it would be more beneficial to start with the simpler optimization, than go the more complex one.

        I guess our approaches are different. I am a student and currently my ultimate goal here is to learn. I think this is achieved better going from simpler things to more complex ones. According to your approach, if I were given a black box that solves my problem in a split of a second, I should have used it and be over with it. This will solve the problem obviously, but will teach me nothing. Anyway, I'm sorry to hear that you think I'm "bordering on irrational obsession". This was actually quite offending.

        UPDATE: re. your update, I did. Quite surprisingly, it now takes significantly longer to run. I guess I have some error, but I can't see it.

        Inline version:

        use strict; use warnings; use List::Util qw(max min); use Time::HiRes qw(time); # this builds a structure that is usually retrieved from disk. # in this example we will use this structure again and again, # but in the real program we obviously retrieve a fresh structure # at each iteration my $simulation_h = {}; for ( 1 .. 70000 ) { my $random_start = int( rand(5235641) ); my $random_length = int( rand(40000) ); push @{ $simulation_h->{$random_start} }, $random_length; } my $zone_o = { _chromosome_length => 5235641, _legal_range => [ { FROM => 100000, TO => 200000 } ] }; my $start_time = time; scenario(); print "total loop time: " . ( time - $start_time ) . " seconds\n"; my $temp_gene_to_legal_range; my $gene_to; sub scenario { for ( my $i = 0 ; $i < 50 ; $i++ ) { print "i=$i time=" . ( time - $start_time ) . " seconds\n"; # originally there was a retreive of $simulation_h from disk h +ere # iterate genes foreach my $gene_from ( keys %{$simulation_h} ) { foreach my $gene_length ( @{ $simulation_h->{$gene_from} } + ) { ### inlining gene_to_legal_range $gene_to = ( ( $gene_from + $gene_length - 1 ) % ( $zone_o->{_chromosome_length} ) ) + 1; if ( $gene_to < $gene_from ) { # split # low range first $temp_gene_to_legal_range = [ { FROM => 0, TO => $gene_to }, { FROM => $gene_from, TO => $zone_o->{_chromosome_length} } ]; } else { # single $temp_gene_to_legal_range = [ { FROM => $gene_from, TO => $gene_to } ]; } } } } }
        21 seconds

        Previous version (with subroutine call):

        use strict; use warnings; use List::Util qw(max min); use Time::HiRes qw(time); # this builds a structure that is usually retrieved from disk. # in this example we will use this structure again and again, # but in the real program we obviously retrieve a fresh structure # at each iteration my $simulation_h = {}; for ( 1 .. 70000 ) { my $random_start = int( rand(5235641) ); my $random_length = int( rand(40000) ); push @{ $simulation_h->{$random_start} }, $random_length; } my $zone_o = { _chromosome_length => 5235641, _legal_range => [ { FROM => 100000, TO => 200000 } ] }; my $start_time = time; scenario(); print "total loop time: " . ( time - $start_time ) . " seconds\n"; my $temp_gene_to_legal_range; my $gene_to; sub scenario { for ( my $i = 0 ; $i < 50 ; $i++ ) { print "i=$i time=" . ( time - $start_time ) . " seconds\n"; # originally there was a retreive of $simulation_h from disk h +ere # iterate genes foreach my $gene_from ( keys %{$simulation_h} ) { foreach my $gene_length ( @{ $simulation_h->{$gene_from} } + ) { #### inlining gene_to_legal_range # $gene_to = # ( ( $gene_from + $gene_length - 1 ) # % ( $zone_o->{_chromosome_length} ) ) + 1; # # if ( $gene_to < $gene_from ) { # # # split # # low range first # $temp_gene_to_legal_range = [ # { FROM => 0, TO => $gene_to }, # { # FROM => $gene_from, # TO => $zone_o->{_chromosome_length} # } # ]; # } # else { # # # single # $temp_gene_to_legal_range = # [ { FROM => $gene_from, TO => $gene_to } ]; # } # # next; #### $temp_gene_to_legal_range = gene_to_legal_range( $gene_from, $gene_length, $zone_o->{_chromosome_length} ); } } } } sub gene_to_legal_range($$$) { return; my ( $gene_from, $gene_length, $legal_length ) = @_; my $ret; my $gene_to = ( ( $gene_from + $gene_length - 1 ) % ($legal_length +) ) + 1; if ( $gene_to < $gene_from ) { # split # low range first $ret = [ { FROM => 0, TO => $gene_to }, { FROM => $gene_from, TO => $legal_length } ]; } else { # single $ret = [ { FROM => $gene_from, TO => $gene_to } ]; } return $ret; }
        7.5 seconds