RE: [code-review] list scope

> -----Original Message-----
> From: [email protected] [mailto:[email protected]]
> Sent: Friday, September 12, 2003 4:17 AM
> To: Hodges, Paul; code-review-ladder-wool9L35kiek0rBIEb6pKdBc4/[email protected]
> Subject: RE: [code-review] list scope

Thanks for the response. :)

> I would have thought that showing any code would be fine. 
> Everyone still learns from the analysis even if they never have a use for
the code.

That was my thinking, but it seemed worth asking before putting deep dark
tangles of pre-mangled code up on the list. I'll start with subsections in
the meantime.

> I've run into something similar to your problem before.
> If you end up running various tweaked analyses on the same
> big, hard to parse files you may get great benefit from
> preparsing them into a faster to parse format.
> You could use Storable for instance but for maximum speed I 
> think you won't beat pack/unpack.

I tried several versions, but unpack seemed the best option in a pure Perl
solution.
I think the real slowdown is the process of putting it on the object,
though:

sub parse { no warnings; # to quiet > & == on 0A's (special account types)
    (*self,$rec) = @_;   # these are our() 
    my $rt = substr($rec||=$self{rec},0,2) or confess "invalid usage";
    if ($rt > 1) { # all greater than 01 is usage, most common type
        @self{@usg_fields} = unpack $usg_layout, $rec;
        $self{usg}         = $rec; # save the usage level raw record
    } elsif($rt == 1) { # 01 records are line data, next most common
        @self{@linefields} = unpack $linelayout, $rec;
        @self{@usg}        = ();   # line cleans out previous usage
        $self{line}        = $rec; # save the line level raw record
    } else {       # 0[0A] is account data, only other types
        @self{@acctfields} = unpack $acctlayout, $rec;
        @self{@clean}      = ();   # account cleans out line & usage data
        $self{acct}        = $rec; # save account level raw record
    }
    # should return $rec (maybe better to explicitly return $self?)
}

@usg_fields, @linefields, @acctfields, @usg, and @clean predefined for
readability and hopefully a little more speed. I'm not sure of the details
on this, but if that isn't faster, I'm assuming it doesn't matter. If it
does, I probably just need to go back to the C module version and tweak
there anyway.

> It's a bit of work to split your process into a parse and dump phase,
> followed by an undump and analyse phase but if you end up reanalysing even
> once, it sounds like you'll get that time back pretty quickly.

Boss doesn't want us rewriting the file format -- we have over a decade of
legacy code reading the current format that *isn't* going to be rewritten to
a new one, so I have to say I agree with that one. :/  Accordingly, we're
stuck with the file format as-is. 

The files are around a GB each, and there are 12, and we have to store over
a year's worth on limited disk space, so we keep them gzip'd. We open gzcat
pipes in our programs to decompress on the fly as an internal input stream,
and usually output to compression pipes as well, so the data never touches
the disk unzipped. All account data is stored on an account record, and all
data following until the next account record is associated with that
account, but there's nothing on the subsequent records to say so except
sequence, so we can't do any sorts on the files. There are several
subcategories as well. It's a structure that begs for an object to store
less granular data, and to automate cleaning that storage out at the next
account.

> Of course improving the parsing sounds like it would be worth doing too.
> In that same project I made good gains also by switching some method
> calls inside tight loops to be function calls. Of course this is not
> something to do without careful thought but I had some methods that were 
> never called by anything outside their own class and so I just turned them

> into functions.

Done that where I could, but the main problem is that I need to be able to
tell the method I want so-and-so to be the current record now, which is
usually going to be by saying "here, parse this one".

> Finally, putting loops _inside_ methods rather than around 
> them can be a big help. So convert from
> 
> foreach my $line (@lines) {
>   push(@parsed, $parser->parse_line($line));
> }
> 
> to
> 
> push(@parsed, $parser->parse_lines(@lines));
> 
> and if you need it you can just reimplement parse_line as a 
> wrapper around parse_lines.

Absolutely -- done that, too. :)

> If @lines is enormous or is coming in from a file then split them into
> batches and you'll still see a speed up.
> Obviously there's not much benefit if parse_lines needs 5 
> minutes per line,
> F

I switched from reading one record on one method call and pasing on a
subcall to reading a MB and splitting it into an array or records on one
call, and having each request for another record just advance through that
array (reloading on the fly at need), but I still have to parse each record
onto the object. I did all that before posting. Nothing seems to really help
enough to matter. :(

sub blockread { my ($self,$buf) = shift;
    my $read = read($self->{_fh},$buf,1105920) or return;   # 54 bytes recs
    push @{ $self->{_recBlockBuffer} }, split /\r\n/, $buf; # CRLF
terminated
    $read;
}

sub nextrec {
    *self   = $_[0];
    *block  = $self{_recBlockBuffer};
    my $ndx = $self{_block_ndx}++;
    if ($ndx > $#block) {
        @block = ();      # clean out the old
        $_[0]->blockread; # read in the new
        $ndx = 0;         # point at the fresh data
    }
    $rec = $self{rec} = $block[$ndx];
}

I wanted to make that block assignment in blockread() be 
    $self->{_recBlockBuffer} = [ split /\r\n/, $buf ];
but there is almost certainly a few records left over from any attempt to
read an account at a time using 

sub readacct {
    *self   = $_[0];
    *block  = $self{_recBlockBuffer} ||= [ ];
    my $ndx = $self{_block_ndx}      ||=  0;
    SCAN:{ $ndx++ until not $block[$ndx] or $ndx and $block[$ndx] =~
/^0[0A]/;
        unless ($block[$ndx]) {
            last SCAN unless $_[0]->blockread; # read new, bail if eof
            redo SCAN; # continue until we have the whole account
        }
    }
    $self{_recBlockBuffer} = [ @block[$ndx..$#block] ]; # block sans acct
    $self{_block_ndx}      = 0;                         # reset to top
    [ @block[0..$ndx-1] ];                              # return accountref
}

readacct() isn't something I expect to see used often, but it's been needed
before....


*****
"The information transmitted is intended only for the person or entity to
which it is addressed and may contain confidential, proprietary, and/or
privileged material.  Any review, retransmission, dissemination or other use
of, or taking of any action in reliance upon, this information by persons or
entities other than the intended recipient is prohibited.  If you received
this in error, please contact the sender and delete the material from all
computers."