Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I am trying to read a single-columned CSV of doubles into Java with a string header. It is 11 megabytes and takes over 15 minutes to read, which is clearly unacceptable. In R this CSV would take about 3 seconds to load.

This CSV file may contain strings so I am parsing it with this in mind.

The CSV reading method needs to return Vector<Double> due to reliance on this output by other parts of the application.

The issue is not due to the isNumber static method, since each call to that is taking 200 nanoseconds, thus contributing approximately 0.2 seconds to the 15 minutes of parsing time.

The Double.valueOf() only takes about 500 nanoseconds, so it is not that either.

csvData.add() is only taking 80 nanoseconds so it is not that.

private static Vector<Double> readTXTFileSingle(String csvFileName) throws IOException {
    String line = null;
    BufferedReader stream = null;
    Vector<Double> csvData = new Vector<Double>();

    try {
        stream = new BufferedReader(new FileReader(csvFileName));
        while ((line = stream.readLine()) != null) {
            String[] splitted = line.split(",");
            if( ! NumberUtils.isNumber(splitted[0])) {

                continue;
            }

            Double dataLine = Double.valueOf(splitted[0]);
            csvData.add(dataLine);

        }
    } finally {
        if (stream != null)
            stream.close();
    }
    return csvData;
}
share|improve this question
2  
How is NumberUtils.isNumber implemented? –  Simon André Forsberg Feb 4 at 13:35
    
@SimonAndréForsberg This is responsible for 200 milliseconds of the lag (200 nanoseconds per call, 1,000,000 times). So it is not the problem –  user2763361 Feb 4 at 14:03
    
I'd still like to see it, if you're willing to provide it. –  Simon André Forsberg Feb 4 at 14:23
1  
Wait a moment, you profiled it but do not know where it is slow? –  Bobby Feb 4 at 14:35
1  
Run a profiler on it...I mean, yes, we are doing reviews for performance improvements, but we do not do guess work. Please run it through a profiler and see what takes that long, then add that information to the question. –  Bobby Feb 4 at 14:38
show 8 more comments

1 Answer

up vote 6 down vote accepted

As per Bobby's comment, Vector is your problem, but not for the reason he says...

Vector is a synchronized class. Each call to any method on Vector will lock the thread, flush all cache lines, and generally waste a lot of time (in a situation where it's usage is in a single thread only).

The fact that you use Vector indicates that you are running some really old code, or you have not properly read the JavaDoc for it.

A secondary performance problem is that each value is being converted to a Double Object. In cases where you have large amounts of data, and where there is a primitive available for you to use, it is always faster to use the primitive (in this case, double instead of Double).

You should also be using the Java7 try-with-resources mechanism for your stream.

My recommendation is to change the signature of your method to return a List... actually, no, my recommendation is to return an array of primitive double[].... if you are interested in speed, this will be a significant improvement:

private static double[] readTXTFileSingle(String csvFileName) throws IOException {
    double[] csvData = new double[4096]; // arbitrary starting size.
    int dcnt = 0;

    try (BufferedReader stream = new BufferedReader(new FileReader(csvFileName))) {
        String line = null;
        while ((line = stream.readLine()) != null) {
            String[] splitted = line.split(",");
            if( ! NumberUtils.isNumber(splitted[0])) {

                continue;
            }

            double dataLine = Double.parseDouble(splitted[0]);
            if (dcnt >= csvData.length) {
                // add 50% to array size.
                csvData = Arrays.copyOf(csvData, dcnt + (dcnt / 2));
            }
            csvData[dcnt++] = dataLine;

        }
    }
    return Arrays.copyOf(csvData, dcnt);
}

Edit:

One other thing, if you want another tweak in performance, use:

String[] splitted = line.split(",", 2);

since you never access more than the first field in the record, you do not need to look for comma's beyond the first comma

share|improve this answer
    
Thank you.!!!!! –  user2763361 Feb 4 at 15:16
2  
"Vector is a synchronized class." Ah, now I understand that odd "You shouldn't use that but I can't remember why" feeling I had. –  Bobby Feb 4 at 15:32
1  
I doubt Vector's synchronization is responsible for a 15 minute run on an 11M file. I've tried this code (with Vector in place) on a local file and it consistently returned in under 1 second. –  bowmore Feb 5 at 7:42
    
@bowmore you are right... by the way, did you run it with a pre-set vector size or did you let it grow? The OP must have something else going on –  rolfl Feb 5 at 11:20
    
I've used the code as is in OP. I only supplied a very naive isNumber() implementation myself by basically calling Double.parseDouble() and catching the exception. –  bowmore Feb 5 at 12:06
show 2 more comments

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.