How can this perl script be optimized/made more idiomatic?

Question

I need to split data found in one file into other files based upon their job id. The structure of the file, and the two helper files, are:

# data.txt - data file - node,timestamp,data
1,1516,25
2,1845,24
3,1637,26
4,1342,74
5,1426,63
6,1436,23
7,1732,64
1,1836,83
2,1277,12
3,2435,62
4,2433,47
5,2496,52
6,2142,69
7,2176,53

# job.txt - job timing - job,startts,endts
1234,1001,2000
5678,2001,2500

# node.txt - node to job map - job,node
1234,1
1234,2
1234,3
1234,4
1234,5
5678,3
5678,4
5678,5
5678,6
5678,7

In order to map a line in the data file to its appropriate new file, two transformations must take place. First, the data timestamp must be used to determine which jobs are running. Second, the list of running jobs must be checked to determine which owns the node the data references. This is my solution:

use strict;
use warnings;

my @timing = ( );
my %nodes = ( );
my %handles = ( );

### array of arrays containing job, start time, and end time
open JOB, "<job.txt" or die "can't open jobfile, $!";
while (<JOB>) {
    my @fields = split /,/;  #/ stop SO highliter
    my @array = ($fields[0], $fields[1], $fields[2]);

    push @timing, \@array;
}
close JOB;

### map job -> array of nodes
open NID "<node.txt" or die "can't open nidfile";
while (<NID>) {
    my @fields = split /,/;  #/

    if (!exists $nodes{$fields[0]}) { $nodes{$fields[0]} = (); }
    push @{$nodes{$fields[0]}}, $fields[1];
}
close NID;

### split data
open DATA, "<data.txt" or die "Couldn't open file all.pow, $!";
while (<DATA>) {
    my @fields = split /,/;  #/

    my @jobs = grep {$fields[1] >= $_->[1] && $fields[1] <= $_->[2]} @timing;
    scalar @jobs > 0 or next;
    my $jid = (grep {!exists $nodes{$fields[0]}} @jobs)[0][0];

    ### create and memoize file handles
    if (!exists $handles{$jid}) {
        open my $temp, ">$jid.txt" or die "Can't open jfile $jid, $!";
        $handles{$jid} = $temp;
    }
    print {$handles{$jid}} "$fields[1],fields[2]";
}
close DATA;

I would like to know if there are any ways to increase the speed/efficiency of the data file loop. This has to run over large amounts of data, and so it needs to be as efficient as possible. I would also appreciate any comments on more idiomatic approaches: this is my first perl script (the array of references to arrays took quite a while to figure out).

Answer 1

As for efficiency, if this code writes to files often (and needs to open many on the fly) I don't see a way to speed it up significantly. Allowing for pre- or post-processing may help but that depends on wider context.

Here is a quick scoop of some coding points. In order of appearance

• There is exactly no use of "initializing" arrays and hashes to an empty list; just declare them

• Always use lexical filehandles and three-argument open (like you do for writing). So

    open my $job_fh, '<', ...
  

• There is no need to first create a key in order to assign to it. The first line below is unneeded

    #if (!exists $nodes{$fields[0]}) { $nodes{$fields[0]} = (); }  # NOT needed
    push @{$nodes{$fields[0]}}, $fields[1];
  

  This is by courtesy of autovivification: if an object is dereferenced in an lvalue context (when it's modifiable) it is created if it is undefined. This is a complex feature. Close to your use is this post; also see this Effective Perler article, and this post with its links.

• Use array, or list, slices. Instead of

    my @array = ($fields[0], $fields[1], $fields[2]);
  

  one can do

    my @array = @fields[0,1,2];   # or @fields[0..2] in this case
  

  Then one can also do

    push @timing, [ @fields[0..2] ];
  

  or

    push @timing, [ (split /,/)[0..2] ];  # no need for @fields now
  

  which are more efficient since intermediate arrays aren't created. But these last two are less flexible, limiting possibilities for intermediate processing or checks. If this is all you do then

    my @timing = map { [ (split /,/)[0..2] ] } <$job_fh>;
  

  also goes, but it is even less flexible (and reads all lines at once^†)

• Arrays in scalar context are evaluated to return the number of elements. So

    scalar @jobs > 0 or next;  # scalar unneeded
  

  does not need scalar and you can write

    @jobs > 0 or next;
  

  which reduces to

    @jobs or next;
  

  but what I'd rather write as

    next if not @jobs;   # or:  next unless @jobs;
  

  using statement modifier ("postfix syntax")

---

^† The <> operator is in list context (see link above) imposed by map so it returns all lines