java servlet : how to speed this up?

Question

I have the following function which is called for every line item produced. Does anyone have any ideas how to speed this up?

private String getDetails(String doc){
    String table="";
    java.sql.ResultSet rs = qw.DBquery("select " +
        "id,LineType, QtyTotal, ManufacturerPartNumber, Description, UnitCost,UnitPrice " +
        "From DocumentItems  " +
        "where DocID="+doc+" order by linenumber " +
        "");
    table+= "<table class=inner><thead><colgroup><col id='col1'><col id='col2'><col id='col3'><col id='col4'><col id='col5'></colgroup>" +
            "<tr class='enetBlue'><th>Qty</th><th>Part Num</th><th>Description</th><th>Unit Cost</th><th>Unit Price</th></tr></thead>" +
            "<tbody>";
    try{            
        int odd = 0;
        while(rs.next()){

            int lineType = rs.getInt("LineType");
            int qty = rs.getInt("QtyTotal");
            String part = rs.getString("ManufacturerPartNumber");
            String desc = rs.getString("Description");
            float cost = rs.getFloat("UnitCost");
            float price = rs.getFloat("UnitPrice");
            String id = rs.getString("id");

            String clas="";

           if (odd==0) odd=1; else odd=0;

           clas="red";
           if (lineType==2) clas="yellow";
           if (lineType==3) clas="yellow";
           if (lineType==4) clas="yellow";
           if (qty==0) clas="yellow";
           java.sql.ResultSet rs2 = mas.DBquery("select itemkey from timitem where itemid = '"+part+"'"); 
           while (rs2.next())
           {
               if (odd==1) clas="odd";
               if (odd==0) clas="even";
           }
           table+="<tr class='"+clas+"'><td>"+qty+"</td>\n"+
                        "<td>"+part+"</td>\n"+
                        "<td>"+desc+"</td>\n"+
                        "<td>"+cost+"</td>\n"+
                        "<td>"+price+"</td></tr>\n";

                       //if clas=red | means item is not found in MAS, gear for insert. 
                       if (clas=="red") { 

                        table+="<tr ><td colspan=5><table border=1><tr><td colspan=2>\n";
                        //get unit measure key  
                        try {
                         table+="<form name=masinsert"+id+" method=get action=MASInsert>\n";

        table+="<input type=hidden name=\"partnumber"+id+"\" value=\""+part+"\">\n";
        table+="<input type=hidden name=\"itemcost"+id+"\" value=\""+cost+"\">\n";
        table+="<input type=hidden name=\"itemlistprice"+id+"\" value=\""+price+"\">\n";
        table+="<input type=hidden name=\"itemdescription"+id+"\" value=\""+desc+"\">\n";
        table+="</td><tr>\n";

                   java.sql.ResultSet rsUM = mas.DBquery("select * from tciUnitMeasure where companyid like 'ENS' ");
                                table+="<tr bgcolor=#990033><td align=left valign=top>Unit Measure</td><td align=left valign=top><select name=\"UnitMeasKey\">";
                                     while(rsUM.next())
                    {
                     table+="<option value=\"" + rsUM.getString("UnitMeasKey") + "\">" + rsUM.getString("UnitMeasID") + "</option>\n"; 
                   }//end while rs1 
                table+="</select></td></tr>\n"; 


           //build ItemClass options from mas: Puchase ProductLine 
               java.sql.ResultSet rsPP = mas.DBquery("select * from timPurchProdLine where companyID = 'ENS'");
               int k = 0; 

               table+= "<tr bgcolor=#990033><td align=left valign=top>Purchase Product Line</td><td align=left valign=top><select name=\"PurchProdLine\">\n"; 
                  while(rsPP.next())
                  {
                   table+="<option value=\"" + rsPP.getString("PurchProdLineKey") + "\">" + rsPP.getString("Description") + "</option>\n"; 

               }//end while rsPP 
               table+="</select></td></tr>\n"; 

               //build item classkey options
               java.sql.ResultSet rsIC = mas.DBquery("select * from timItemClass where companyID = 'ENS' order by itemclassname desc");

               table+= "<tr bgcolor=#990033><td align=left valign=top>Item Class :</td><td align=left valign=top><select name=\"itemclasskey\">\n"; 
                  while(rsIC.next())
                  {
                   table+="<option value=\"" + rsIC.getString("itemclasskey") + "\">" + rsIC.getString("ItemClassName") + "</option>\n"; 

               }//end while rs1 
               table+="</select></td></tr>";
               table+="<tr><td colspan=2><input id='m"+id+"' type=\"button\" onclick=\"masinsert('"+ id +"')\" value=\"Add to MAS\"></td></tr>";
               table+="</table>\n"; 

       }catch(Exception e){} //end try

           table+="</form>\n"; 
                        table+="</td></tr>";


             }//end if clas=red
            }//end while 
    }catch(java.sql.SQLException e){
        e.printStackTrace();}        
    table+="</tbody></table>";
    return table;
}

thanks in advance

Answer 1

Appending Strings with the '+' operator is very slow, because the JVM needs to iterate over each character in the String for each append, due to the immutability of Strings.

Look into StringBuilder

You may also be able to use SQL JOINs as opposed to having to iterate over each record and formulate new queries. You would essentially only need to hit the database once.

You also have a query inside your loops that always returns the same data:

 java.sql.ResultSet rsUM = mas.DBquery("select * from tciUnitMeasure where companyid like 'ENS' ");

This should be moved outside of the loop, though I suspect you may be able to eliminate this and the other nested queries using a JOIN.

Answer 2

I would cache the output of the function keyed on the doc parameter. Depending on the app usage patterns it can speed up your app tremendously.

Answer 3

Learn how to write JSPs and stop embedding HTML and CSS in servlets. That was a bad idea back in 1998, when it was common; we've progressed a long way since.

When you loop over one ResultSet and execute a query inside the loop, that means a network round trip for each and every row that you bring back with the first query. That's certain to be the biggest bottleneck in your problem. Eliminate that for starters.

I'd recommend moving the database code out of the servlet and into a persistence object that you can test without having to fire up the servlet engine.

There's nothing object-oriented about this approach at all. Looks like an Invoice or BillOfMaterials, but I don't see any objects.

Answer 4

Use a precompiled parametrized PreparedStatment rather then building it with String concatenation everytime. This will also fix the fact that your current code (if doc is a user entered variable) is susceptible to SQL Injection attacks.

Answer 5

You have a select * from timPurchProdLine where companyID = 'ENS' line that you run in each iteration. Will the contents of that table change with every iteration? Because if they're static, you will save A LOT of time if you only read the contents of that table once, before your loop, creating the substring you need. Then you just append that string inside your loop when you need it. The same goes for the select * from timItemClass where companyID = 'ENS' order by itemclassname desc query.

Don't use "+" to concat strings. Use StringBuilder or if you're using Java 5 or 6, use String.format(). Also, I think it's faster to use the ResultSet methods that take a column number instead of the column name for a parameter, that is, getString(1) is faster than getString("id").

Stuff like odd = 1-odd will save a couple of CPU cycles but it's probably not going to be a noticeable improvement; but it is even more understandable if you use a boolean and do odd = !odd instead of the if (odd==0) odd=1; else odd=0 stuff.

Answer 6

I don't get the second query at all.

 java.sql.ResultSet rs2 = mas.DBquery(...); 
 while (rs2.next())
 {
   if (odd==1) clas="odd";
   if (odd==0) clas="even";
 }

You perform the query and iterate across it but you don't get anything out of rs2, you set clas based on the value of odd, but don't modify the value of odd inside the loop, meaning that there's no point int he while loop at all, much less the query. Further, if the query has no results, the even / odd logic never gets run at all.

EDIT:

The 'odd' handling is odd. Use a boolean...

boolean odd = false;
...
odd = !odd; // instead of your if statement
... 
clas = odd ? "odd" : "even"; // inside the while loop.

Your code includes

String clas = "";
...
clas = "red";

Declare clas near first use and initialize it with a reasonable default value.

// String clas = ""; remove this line
...
String clas = "red";

This line is bad

if (clas == "red") {

First off, doing a reference compare with strings is bad form. I use the idiomn

if ("red".equals(clas))

But even then you're better off storing a boolean as to whether you've found a value and testing against it rather than a string compare.

java.sql.ResultSet rsPP = mas.DBquery("select * from timPurchProdLine where companyID = 'ENS'");

As stated elsewhere, doing a bunch of queries inside a loop that do not depend on loop variables is a huge waste. Do all the queries before hand and cache the resulting values or HTML.

Answer 7

Here is a bit of your code extracted into its own function and adapted to use StringBuffer and String.format().

private String timItemClassTable() throws SQLException {
    StringBuffer sb = new StringBuffer();
    ResultSet rs = mas.DBquery("select * from timItemClass where companyID = 'ENS' order by itemclassname desc");
    sb.append("<tr bgcolor=#990033><td align=left valign=top>Item Class :</td><td align=left valign=top><select name=\"itemclasskey\">\n");
    while (rs.next())
     sb.append(String.format("<option value=\"%s\">%s</option>\n", rs.getString("itemclasskey"), rs.getString("ItemClassName")));
    sb.append("</select></td></tr>");
    return sb.toString();
}

Having smaller methods like this makes it easier to find the bottlenecks in your code and easier to adjust the code to make it faster (for instance by moving the method calls outside the loop where possible). Smaller methods are also easier to test and debug.

This refactoring of your code will not in itself make your code faster, but it will make your code easier to work with, it will make it easier for you to make your code faster.

Answer 8

Please use a template engine, what are you doing there it's a terrible crime :)

I would recommend Apache Velocity , it is very easy to iterate in any java application... and of course try to optimize the way how you retrieve your data...

Answer 9

There are a number of improvements on the code.

I'll try to address some of them, the most relevant.

• The most relevant. This should use jsp/servlet/beans.

  But since the code is not structured like that, I won't explain further. Better is to explain the improvements to the servlet as it is.

• The method is doing too many things, in one place. To make it better several utility methods are needed, while this don't improve performance them selves, definitely help to maintain the code and make clearer what's the method purpose ( allowing modify the code for improvements.

• Some parts of the code doesn't change at all. They should be declared as constants. For instance there are three combos that never change. They should be in a constant, that way, it doesn't matter if the code use String "+" because they are invoked only once.

• Open nested resulsets. Before opening and consuming a ResultSet the previous should be consumed and closed. This way the connections have the opportunity to save db resources.

• qw.DBquery Perhaps the real-real problem in this code would this object. I don't know what the code look like, but probably it doesn't use a connection pool nor it close the connection each time a query is performed. This could potentially means that for the given code up to 5 simultaneous connections could have been opened. While only one is needed. Even worst the next time ( if the connections are not closed ) another new 5 connections could be used, when probably a single connection could do all the work for several hours. We never know.

Beyond the replacement of String concatenation using "+" I think the db resource usage is what's killing the performance of this servlet.

Finally here's my attempt to do a major refactor of the code.

I hope this helps to understand better how a java application should be done. I didn't compile it, so It won't work. This is only done to show up how the above items could be fixed.

import java.sql.ResultSet;
import java.util.List;
import java.util.ArrayList;
// etc etc. etc

First of all, there are a number of strings that could be declared as constants.

public class SomeServlet extends HttpServlet {


    // ALL THE FOLLOWING ARE CONSTANTA IN THE SERVLET.
    // THEIR VALUE DOES'T CHANGE DURING EXECUTION
    private static final String SELECT_DOCUMENT_QUERY =
                                   "SELECT \n" +
                                  "   id,LineType, QtyTotal, ManufacturerPartNumber, Description, UnitCost,UnitPrice \n" +
                                  " FROM DocumentItems  \n" +
                                  " WHERE DocID=%s order by linenumber ";

    private static final String HTML_HEADER_TABLE =
                     "<table class=inner><thead><colgroup><col id='col1'>"+
                     "<col id='col2'><col id='col3'><col id='col4'><col id='col5'></colgroup>" +
                     "<tr class='enetBlue'><th>Qty</th><th>Part Num</th><th>Description</th>"+
                     "<th>Unit Cost</th><th>Unit Price</th></tr></thead><tbody>";


    // These constants are the HTML Commbo for the given queries. 
    // If they are initialized once at the beginning will increase
    // the servlets performace considerabily.
    private static final String UNIT_MEASURES_COMBO = 
                      getCombo( "Unit Measure", "UnitMeasKey", 
                          getListFrom( "select * from tciUnitMeasure where companyid like 'ENS' ") );

    private static final String ITEM_CLASS_COMBO    = 
                      getCombo( "Purchase Product Line", "PurchProdLine", 
                          getListFrom( "select * from timPurchProdLine where companyID = 'ENS' ") );

    private static final String CLASS_KEY_COMBO     = 
                      getCombo( "Item Class :", "itemclasskey", 
                          getListFrom( "select * from timItemClass where companyID = 'ENS' order by itemclassname desc") );

Then, although this is definitely not the right way, a servlet may define some beans to help him to do the job, we could create this classes like this:

    // Use a class to separete domain objects from presentation
    // This class becomes handy do pass data through methods. 
    class Document {
        int lineType;
        int qty;
        String part;
        String desc;
        float cost;
        float price;
        String id;
    }
    // simple clas that holds a key/value pair
    class ComboPair {
        String key;
        String value;
        public ComboPair( String key, String value ) { 
            this.key = key;
            this.value = value;
        }
    }

Finally the refactored code. When a method is doing too many things it could delegate the work to others. When two pieces of code look the same, they should use an auxiliary method.

    /*
    * Finally the fixed code.
    *
    * Basically a method should do only one thig. If the method is doing a lot of things 
    * several functionality at the same time, it should delegate the details of the subfunctionality 
    * to another function. This way each function or method is easier to read/maintain/understand.
    * This method should read like this:
    * 
    * For a given docId, 
    *    - query the database and display details of the document
    *    - if the document is not present in mas?
    *         - create a form to inser it
    *  
    * differntiate each record with different style.
    */
    private String getDetails(String doc){
        // Close each result set before openning a new one.
        ResultSet rs = qw.DBquery( String.format( SELECT_DOCUMENT_QUERY, doc ));
        List<Document> documents = new ArrayList<Document>();
        while(rs.next()){
            documents.add( createDocumentFrom( rs ));
        }
        // Iterate through 
        StringBuilder resultingTable = new StringBuilder( HTML_HEADER_TABLE );
        boolean isEven = false;// starts as odd
        for( Document doc : documents ) { 
            String clazz = getClassFor( doc , isEven );
            isEven = !isEven;

            resultingTable.append("<tr class='"+clazz+"'>"+
                        "<td>"+doc.qty+"</td>\n"+
                        "<td>"+doc.part+"</td>\n"+
                        "<td>"+doc.desc+"</td>\n"+
                        "<td>"+doc.cost+"</td>\n"+
                        "<td>"+doc.price+"</td></tr>\n");

            if( needsInsertForm( clazz ) ) { 
                resultingTable.append( getInsertForm( document ) );
            }

            resultingTable.append("</tbody></table>");
        }
        return table;
    }


    /**
     * This methods craates an instance of "Document". 
     * Instead of mixing the fetch code with the render code
     * this method allows to separete the data from its presentation
     */
    private Document createDocumentFrom( ResultSet rs ) { 
        Document document = new Document();

        document.lineType = rs.getInt("LineType");
        document.qty = rs.getInt("QtyTotal");
        document.part = rs.getString("ManufacturerPartNumber");
        document.desc = rs.getString("Description");
        document.cost = rs.getFloat("UnitCost");
        document.price = rs.getFloat("UnitPrice");
        document.id = rs.getString("id");

        return document;
    }



    // Computes the correct css class for the given document. 
    private static String getClassFor( Document document, boolean isEven ) { 
        String clazz = "red";
        switch( document.lineType ) {
            case 2:
            case 3:
            case 4: clazz ="yellow";
        }
        if( document.qty == 0 ) { 
            clazz = "yellow";
        }
       ResultSet rs = mas.DBquery( String.format("select itemkey from timitem where itemid = '%s'", document.part); 
       while (rs.next()) {
           clazz = isEven? "even" : "odd";
       }
       rs.close();
       return clazz;

    }

    // Creates the inser form for the given document
    private static String getInsertForm( Document document ) { 
        StringBuilder form = new StringBuilder();

        form.append("<tr ><td colspan=5><table border=1><tr><td colspan=2>\n");
        form.append("<form name=masinsert"+document.id+" method=get action=MASInsert>\n");
        form.append("<input type=hidden name=\"partnumber"+document.id+"\" value=\""+document.part+"\">\n");
        form.append("<input type=hidden name=\"itemdescription"+document.id+"\" value=\""+document.desc+"\">\n");
        form.append("<input type=hidden name=\"itemcost"+document.id+"\" value=\""+document.cost+"\">\n");
        form.append("<input type=hidden name=\"itemlistprice"+document.id+"\" value=\""+document.price+"\">\n");
        form.append("</td><tr>\n");
        //----------------------------------------------------------------------------------------------------------------------------          
        //get unit measure key  
        form.append(UNIT_MEASURES_COMBO);                  

        //build ItemClass options from mas: Puchase ProductLine 
        form.append(ITEM_CLASS_COMBO);

        //build item classkey options
        form.append( CLASS_KEY_COMBO);
        //----------------------------------------------------------------------------------------------------------------------------
        form.append("<tr><td colspan=2><input id='m"+document.id+"' type=\"button\" onclick=\"masinsert('"+ document.id +"')\" value=\"Add to MAS\"></td></tr>");
        form.append("</table>\n"; 
        form.append("</form>\n"; 
        form.append("</td></tr>");
        return form.toString();

    }


    // This is an utility method that reads bettern when used in the 
    // main method.
    // if( needsInsertForm( clazzz ) ) {
    // is much clearer
    private static boolean needsInsertForm( String clazz ) { 
        return "red".equals(clazz);
    }

There are two methods not included here. They are still messy, but they help to create the constants. That way instead of creating those strings each time, they are just created once during the servlet life.

The complete code is here http://pastebin.com/f2ad1510d

I hope it helps.

Answer 10

This is a good time for learning to use a profiler. This will tell you where the time is actually spent, so you know what to look at for performance improvement.

If you use the latest java 6 then I recommend that you look into visualvm. (jvisualvm in the JDK bin directory).

It can profile an already running java program!

Answer 11

Get the queries out of the loop. Join all the necessary tables outside the loop. This alone will give you the best possible performance boost. You might save a few additional nanoseconds by using a StringBuffer resp. StringBuilder.