5
\$\begingroup\$

I have a performance problem when trying to create a csv file starting from another csv file.

This is how the original file looks:

country,state,co,olt,olu,splitter,ont,cpe,cpe.latitude,cpe.longitude,cpe.customer_class,cpe.phone,cpe.ip,cpe.subscriber_id
COUNTRY-0001,STATE-0001,CO-0001,OLT-0001,OLU0001,SPLITTER-0001,ONT-0001,CPE-0001,28.21487,77.451775,ALL,SIP:[email protected],SIP:[email protected],CPE_SUBSCRIBER_ID-QHLHW4
COUNTRY-0001,STATE-0002,CO-0002,OLT-0002,OLU0002,SPLITTER-0002,ONT-0002,CPE-0002,28.294018,77.068924,ALL,SIP:[email protected],SIP:[email protected],CPE_SUBSCRIBER_ID-AH8NJQ

Potentially it could be millions of lines like this, i have detected the problem with 1.280.000 lines.

This is the algorithm:

File csvInputFile = new File(csv_path);
int blockSize = 409600;
brCsvInputFile = new BufferedReader(frCsvInputFile, blockSize);

String line = null;
StringBuilder sbIntermediate = new StringBuilder();
skipFirstLine(brCsvInputFile);
while ((line = brCsvInputFile.readLine()) != null) {
    createIntermediateStringBuffer(sbIntermediate, line.split(REGEX_COMMA));
}


private static void skipFirstLine(BufferedReader br) throws IOException {
    String line = br.readLine();
    String[] splitLine = line.split(REGEX_COMMA);
    LOGGER.debug("First line detected! ");
    createIndex(splitLine);
    createIntermediateIndex(splitLine);
}

private static void createIndex(String[] splitLine) {
    LOGGER.debug("START method createIndex.");
    for (int i = 0; i < splitLine.length; i++)
        headerIndex.put(splitLine[i], i);
    printMap(headerIndex);
    LOGGER.debug("COMPLETED method createIndex.");
}

    private static void createIntermediateIndex(String[] splitLine) {

    LOGGER.debug("START method createIntermediateIndex.");
    com.tekcomms.c2d.xml.model.v2.Metadata_element[] metadata_element = null;
    String[] servicePath = newTopology.getElement().getEntity().getService_path().getLevel();

    if (newTopology.getElement().getMetadata() != null)
        metadata_element = newTopology.getElement().getMetadata().getMetadata_element();

    LOGGER.debug(servicePath.toString());
    LOGGER.debug(metadata_element.toString());

    headerIntermediateIndex.clear();
    int indexIntermediateId = 0;
    for (int i = 0; i < servicePath.length; i++) {
        String level = servicePath[i];
        LOGGER.debug("level is: " + level);
        headerIntermediateIndex.put(level, indexIntermediateId);
        indexIntermediateId++;
        // its identificator is going to be located to the next one
        headerIntermediateIndex.put(level + "ID", indexIntermediateId);
        indexIntermediateId++;
    }
    // adding cpe.latitude,cpe.longitude,cpe.customer_class, it could be
    // better if it would be metadata as well.
    String labelLatitude = newTopology.getElement().getEntity().getLatitude();
    // indexIntermediateId++;
    headerIntermediateIndex.put(labelLatitude, indexIntermediateId);
    String labelLongitude = newTopology.getElement().getEntity().getLongitude();
    indexIntermediateId++;
    headerIntermediateIndex.put(labelLongitude, indexIntermediateId);
    String labelCustomerClass = newTopology.getElement().getCustomer_class();
    indexIntermediateId++;
    headerIntermediateIndex.put(labelCustomerClass, indexIntermediateId);

    // adding metadata
    // cpe.phone,cpe.ip,cpe.subscriber_id,cpe.vendor,cpe.model,cpe.customer_status,cpe.contact_telephone,cpe.address,
    // cpe.city,cpe.state,cpe.zip,cpe.bootfile,cpe.software_version,cpe.hardware_version
    // now i need to iterate over each Metadata_element belonging to
    // topology.element.metadata
    // are there any metadata?
    if (metadata_element != null && metadata_element.length != 0)
        for (int j = 0; j < metadata_element.length; j++) {
            String label = metadata_element[j].getLabel();
            label = label.toLowerCase();
            LOGGER.debug(" ==label: " + label + " index_pos: " + j);
            indexIntermediateId++;
            headerIntermediateIndex.put(label, indexIntermediateId);
        }

    printMap(headerIntermediateIndex);
    LOGGER.debug("COMPLETED method createIntermediateIndex.");
}

Reading the entire dataset, 1.280.000 lines take 800 ms! so the problem is in this method

    private static void createIntermediateStringBuffer(StringBuilder sbIntermediate, String[] splitLine) throws ClassCastException,
        NullPointerException {

    LOGGER.debug("START method createIntermediateStringBuffer.");
    long start, end;
    start = System.currentTimeMillis();
    ArrayList<String> hashes = new ArrayList<String>();
    com.tekcomms.c2d.xml.model.v2.Metadata_element[] metadata_element = null;

    String[] servicePath = newTopology.getElement().getEntity().getService_path().getLevel();
    LOGGER.debug(servicePath.toString());

    if (newTopology.getElement().getMetadata() != null) {
        metadata_element = newTopology.getElement().getMetadata().getMetadata_element();
        LOGGER.debug(metadata_element.toString());
    }

    for (int i = 0; i < servicePath.length; i++) {
        String level = servicePath[i];
        LOGGER.debug("level is: " + level);
        if (splitLine.length > getPositionFromIndex(level)) {
            String name = splitLine[getPositionFromIndex(level)];
            sbIntermediate.append(name);
            hashes.add(name);
            sbIntermediate.append(REGEX_COMMA).append(HashUtils.calculateHash(hashes)).append(REGEX_COMMA);
            LOGGER.debug(" ==sbIntermediate: " + sbIntermediate.toString());
        }
    }

    //      end=System.currentTimeMillis();
    //      LOGGER.info("COMPLETED adding name hash. " + (end - start) + " ms. " + (end - start) / 1000 + " seg.");
    // adding cpe.latitude,cpe.longitude,cpe.customer_class, it should be
    // better if it would be metadata as well.
    String labelLatitude = newTopology.getElement().getEntity().getLatitude();
    if (splitLine.length > getPositionFromIndex(labelLatitude)) {
        String lat = splitLine[getPositionFromIndex(labelLatitude)];
        sbIntermediate.append(lat).append(REGEX_COMMA);
    }

    String labelLongitude = newTopology.getElement().getEntity().getLongitude();
    if (splitLine.length > getPositionFromIndex(labelLongitude)) {
        String lon = splitLine[getPositionFromIndex(labelLongitude)];
        sbIntermediate.append(lon).append(REGEX_COMMA);
    }
    String labelCustomerClass = newTopology.getElement().getCustomer_class();
    if (splitLine.length > getPositionFromIndex(labelCustomerClass)) {
        String customerClass = splitLine[getPositionFromIndex(labelCustomerClass)];
        sbIntermediate.append(customerClass).append(REGEX_COMMA);
    }
    //      end=System.currentTimeMillis();
    //      LOGGER.info("COMPLETED adding lat,lon,customer. " + (end - start) + " ms. " + (end - start) / 1000 + " seg.");
    // watch out metadata are optional, it can appear as a void chain!
    if (metadata_element != null && metadata_element.length != 0)
        for (int j = 0; j < metadata_element.length; j++) {
            String label = metadata_element[j].getLabel();
            LOGGER.debug(" ==label: " + label + " index_pos: " + j);
            if (splitLine.length > getPositionFromIndex(label)) {
                String actualValue = splitLine[getPositionFromIndex(label)];
                if (!"".equals(actualValue))
                    sbIntermediate.append(actualValue).append(REGEX_COMMA);
                else
                    sbIntermediate.append("").append(REGEX_COMMA);
            } else
                sbIntermediate.append("").append(REGEX_COMMA);
            LOGGER.debug(" ==sbIntermediate: " + sbIntermediate.toString());
        }//for
    sbIntermediate.append("\n");
    end = System.currentTimeMillis();
    LOGGER.info("COMPLETED method createIntermediateStringBuffer. " + (end - start) + " ms. ");
}

As you can see, this method adds a precalculated line to the StringBuffer, reads every line from input .csv file, calculate new data from that lines and finally add the generated line to the StringBuffer, so finally i can create the file with that buffer.

I have run JConsole and I can see that there are no memory leaks. I can see the sawtooths representing the creation of objects and the garbage-collector recollecting garbage. It never surpasses the memory heap threshold.

One thing I have noticed is that the time needed for add a new line to the StringBuffer is completed within a very few ms range, (5,6,10), but is raising with time, to (100-200) ms and I suspect more in a near future, so probably this is the battle horse.

I have tried to analyze the code. I know that there are 3 for loops, but they are very shorts, the first loop iterates over 8 elements only:

for (int i = 0; i < servicePath.length; i++) {
        String level = servicePath[i];
        LOGGER.debug("level is: " + level);
        if (splitLine.length > getPositionFromIndex(level)) {
            String name = splitLine[getPositionFromIndex(level)];
            sbIntermediate.append(name);
            hashes.add(name);
            sbIntermediate.append(REGEX_COMMA).append(HashUtils.calculateHash(hashes)).append(REGEX_COMMA);
            LOGGER.debug(" ==sbIntermediate: " + sbIntermediate.toString());
        }
    }

I have measured the time needed to get the name from the split line and it is worthless, 0 ms, the same to calculateHash method, 0 ms.

The other loop, are practically the same, iterates over 0 to n, where n is a very tiny int, 3 to 10 for example, so I do not understand why it takes more time to finish the method. The only thing I find is that to add a new line to the buffer is getting slow the process.

I am thinking about a producer consumer multi threaded strategy. A reader thread that reads every line and put them into a circular buffer, another threads take it one by one, process them and add a precalculated line to the StringBuffer, which is thread safe. When the file is fully read, the reader thread sends a message to to the another threads telling them to stop. Finally I have to save this buffer to a file. What do you think? this is a good idea?

\$\endgroup\$
3
  • 1
    \$\begingroup\$ Please show us what you are doing with the StringBuilder sbIntermediate after the file is read. \$\endgroup\$
    – Heslacher
    Dec 4, 2014 at 14:28
  • \$\begingroup\$ @Heslacher, after the file is read, sbIntermediate is saved to a file. \$\endgroup\$
    – aironman
    Dec 4, 2014 at 15:04
  • 1
    \$\begingroup\$ You should not cross-post the question, surely not without linking them. \$\endgroup\$
    – maaartinus
    Dec 4, 2014 at 15:19

2 Answers 2

3
\$\begingroup\$

In addition to @rolfl's answer

To soften the effect mentioned by rolfl, you could use the blocksize for constructing the StringBuilder also like

StringBuilder sbIntermediate = new StringBuilder(blockSize);  

Also by calling often the .toSTring() of the stringbuilder inside the createIntermediateStringBuffer() method this will slow down if the stringbuilders content grow.

You say this loop iterates over 8 elements

for (int i = 0; i < servicePath.length; i++) {
    String level = servicePath[i];
    LOGGER.debug("level is: " + level);
    if (splitLine.length > getPositionFromIndex(level)) {
        String name = splitLine[getPositionFromIndex(level)];
        sbIntermediate.append(name);
        hashes.add(name);
        sbIntermediate.append(REGEX_COMMA).append(HashUtils.calculateHash(hashes)).append(REGEX_COMMA);
        LOGGER.debug(" ==sbIntermediate: " + sbIntermediate.toString());
    }
}  

Hence the sbIntermediate.toString() is called 8 times also. So the toString() method of the StringBuilder calls this constructor of the String class. While "only" doing an ArrayCopy, doing this 8 times for a great char[] will slow the process.

This loop could use some facelifting by removing the Logger stuff and also simplifying the logic.

for (int i = 0; i < servicePath.length; i++) {

    int position = getPositionFromIndex(servicePath[i]);

    if (splitLine.length > position ) {
        String name = splitLine[position];

        hashes.add(name);

        sbIntermediate
          .append(name)
          .append(REGEX_COMMA)
          .append(HashUtils.calculateHash(hashes))
          .append(REGEX_COMMA);
    }
} 

Then you have this loop also

if (metadata_element != null && metadata_element.length != 0)
    for (int j = 0; j < metadata_element.length; j++) {
        String label = metadata_element[j].getLabel();
        LOGGER.debug(" ==label: " + label + " index_pos: " + j);
        if (splitLine.length > getPositionFromIndex(label)) {
            String actualValue = splitLine[getPositionFromIndex(label)];
            if (!"".equals(actualValue))
                sbIntermediate.append(actualValue).append(REGEX_COMMA);
            else
                sbIntermediate.append("").append(REGEX_COMMA);
        } else
            sbIntermediate.append("").append(REGEX_COMMA);
        LOGGER.debug(" ==sbIntermediate: " + sbIntermediate.toString());
    }//for

at least it is called once more if metadata_element.length > 0.

Also this could really need a refactoring.

Let us add braces {} for single if statements and remove the logger stuff and reduce the code itself.

if (metadata_element != null && metadata_element.length != 0) {
    for (int j = 0; j < metadata_element.length; j++) {
        String label = metadata_element[j].getLabel();

        String actualValue = "";
        if (splitLine.length > getPositionFromIndex(label)) {
            actualValue = splitLine[getPositionFromIndex(label)];
        }
        sbIntermediate.append("").append(REGEX_COMMA);
    }
}

As the content of the StringBuilder is later written to a file, we can just return a String by the createIntermediateStringBuffer which we write to the file.

File csvInputFile = new File(csv_path);
int blockSize = 409600;
brCsvInputFile = new BufferedReader(frCsvInputFile, blockSize);

Writer writer = new BufferedWriter(new OutputStreamWriter(
        new FileOutputStream(outputFileName), "utf-8"));
String line = null;
skipFirstLine(brCsvInputFile);
while ((line = brCsvInputFile.readLine()) != null) {
    writer.write(createIntermediateStringBuffer(line.split(REGEX_COMMA)));
}
writer.close();

And the whole method refactored by extracting the code duplication to a separate method

private static String createIntermediateStringBuffer (String[] splitLine) throws ClassCastException,
        NullPointerException {
    StringBuilder sbIntermediate = new StringBuilder(1024);

    ArrayList<String> hashes = new ArrayList<String>();
    com.tekcomms.c2d.xml.model.v2.Metadata_element[] metadata_element = null;

    String[] servicePath = newTopology.getElement().getEntity().getService_path().getLevel();

    for (int i = 0; i < servicePath.length; i++) {

        int position = getPositionFromIndex(servicePath[i]);

        if (splitLine.length > position) {
            String name = splitLine[position];

            hashes.add(name);

            sbIntermediate
                    .append(name)
                    .append(REGEX_COMMA)
                    .append(HashUtils.calculateHash(hashes))
                    .append(REGEX_COMMA);
        }
    }

    String labelLatitude = newTopology.getElement().getEntity().getLatitude();
    addByValue(sbIntermediate, splitLine, labelLatitude);

    String labelLongitude = newTopology.getElement().getEntity().getLongitude();
    addByValue(sbIntermediate, splitLine, labelLongitude);

    String labelCustomerClass = newTopology.getElement().getCustomer_class();
    addByValue(labelCustomerClass, splitLine, labelLongitude);

    if (newTopology.getElement().getMetadata() != null) {
        metadata_element = newTopology.getElement().getMetadata().getMetadata_element();
        for (Metadata_element element:metadata_element) {

            String label = metadata_element[j].getLabel();
            String actualValue = "";
            if (splitLine.length > getPositionFromIndex(label)) {
                actualValue = splitLine[getPositionFromIndex(label)];
            }
            sbIntermediate.append("").append(REGEX_COMMA);
        }
    }

    sbIntermediate.append("\n");
    return sbIntermediate.toString();
}

private static void addByValue(StringBuilder sbIntermediate, String[] splitLine, String value) {
    int index = getPositionFromIndex(value);
    if (splitLine.length <= index) {
        return;
    }
    sbIntermediate.append(splitLine[index]).append(REGEX_COMMA);
}
\$\endgroup\$
3
  • \$\begingroup\$ Hi Heslacher, i have put your code running and it is returning me this:Exception in thread "main" java.lang.OutOfMemoryError: Java heap space at java.util.Arrays.copyOf(Arrays.java:2367) at java.lang.AbstractStringBuilder.expandCapacity(AbstractStringBuilder.java:130) at java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:114) at java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:612) at java.lang.StringBuilder.append(StringBuilder.java:219) \$\endgroup\$
    – aironman
    Dec 4, 2014 at 16:17
  • \$\begingroup\$ i have set -Xmx1024M -XX:MaxPermSize=256m -Xms1024, it is taking so few time to get this exception, almost i have no time to activate jconsole! anyway, i think your refactoring goes to correct direction. \$\endgroup\$
    – aironman
    Dec 4, 2014 at 16:21
  • \$\begingroup\$ Thank you very much, after a sleeping night, now i see what was my error. @Heslacher and the other user that stackexchange does not let me to indicate, thank you very much, i have learn a lot of from you, thanks again. \$\endgroup\$
    – aironman
    Dec 5, 2014 at 9:34
2
\$\begingroup\$

Your issue is almost certainly here:

StringBuilder sbIntermediate = new StringBuilder();
skipFirstLine(brCsvInputFile);
while ((line = brCsvInputFile.readLine()) != null) {
    createIntermediateStringBuffer(sbIntermediate, line.split(REGEX_COMMA));
}

For each line of text in the file, you append data to the sbIntermediate StringBuilder. StringBuilders have an internal char[] array that they use to store the data you append. In this case, you are adding millions of lines to it. As you add more data to it, the system needs to 'grow' the char[] array to store the new data. To grow the array the StringBuilder allocates a new, and larger array, and then throws away the previous one (See the source code for StringBuilder).

As you get larger StringBuilders, you need to do more garbage collection and other memory management. This slows things down a lot.

You need to change your algorithm to process one output line at a time, instead of batching them all up in to a single StringBuilder.

Do you really need to keep all the output in memory at the same time? Can you not write out each line to output when it is complete, just like you read one line at a time from the source?

As an experiment, if you do: sbIntermediate.setLength(0) inside the loop, it will 'clear' each output line after populating it. How much faster does it run?

\$\endgroup\$
2
  • \$\begingroup\$ Hi Rolf, thanks for the response, if i put the sentence to my code, the code is much faster. \$\endgroup\$
    – aironman
    Dec 4, 2014 at 15:42
  • 1
    \$\begingroup\$ No problem. What that shows is that the performance problem is not related to how you process the text, but rather how you store and accumulate it. You need to find way to output the data (to a file, or some other system) instead of loading it all in to memory. \$\endgroup\$
    – rolfl
    Dec 4, 2014 at 15:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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