why is it so slow? need to optimize it?

[skarosg3]

Hi all, i have a technical issue here that i would need some help.
I have this code:

for(...)//we dont care about that
{
   if(rs.next()){
           long start = System.currentTimeMillis();
           int count=calcOd(Home,HW,HD,HL,Away,AW,AD,AL);
           long elapsedTimeMillis = System.currentTimeMillis()-start;
           statement.executeUpdate("insert into times values("+ elapsedTimeMillis/1000+ ","+count+")");
   }
}

That is calling this method

public  int calcOd(String HTeam,int HW,int HD,int HL,String ATeam, int AW,int AD,int AL){
int HT=HW+HD+HL,AT=AW+AD+AL,AG,HG,newGames=0;
float HWR=(HW*100/HT),AWR=(AW*100/HT),HLR=(HL*100/HT),ALR=(AL*100/AT),
        Home=0,Away=0;
float maxA=Float.parseFloat(MaxAText.getText()),
      maxB=Float.parseFloat(MaxAText.getText()),
      maxC=Float.parseFloat(MaxC.getText()),
      minA=Float.parseFloat(MinA.getText()),
      minB=Float.parseFloat(MinA.getText()),
      minC=Float.parseFloat(MinC.getText()),
      stepA=Float.parseFloat(StepA.getText()),
      stepB=Float.parseFloat(StepA.getText()), 
      stepC=Float.parseFloat(StepC.getText()),
        a=maxA,b=maxB,c=maxC; 
String myResult="";
boolean done=false;
int count=0;

String[] games=new  String[5];
try{
    statement    = dbConnection.createStatement();
    rs=statement.executeQuery("select * from engg where ht like '%"+HTeam+"%' and at like '%"+ATeam+"%'");
    rs.next();
  
    String result=rs.getString("HT")+rs.getInt("hs")+rs.getString("AT")+rs.getInt("as");
    
    if ((rs.getInt("hs"))>(rs.getInt("as")))
            result="1";
    else if ((rs.getInt("hs"))<(rs.getInt("as")))
            result="2";
    if ((rs.getInt("hs"))==(rs.getInt("as")))
            result="3";
    games[0]=rs.getString("HT");
    games[1]=rs.getString("hs");
    games[2]=rs.getString("AT");
    games[3]=rs.getString("as");

    while ((a>minA)){
        Home=a*HWR;
        while((b>minB)){
            Away=b*AWR;
            while (c>minC){count++;
             if(Home-Away>c)
                    myResult="1";
                else if(Home-Away<-1*c)
                    myResult="2";
                if((Home-Away>=-1*c)&&(Home-Away<=c))
                    {
                    myResult="3";
                    if(HLR-ALR>0.8) myResult="2";
                }
            if(result.equalsIgnoreCase(myResult))
            {  
               statement.executeUpdate("insert into table3 values("+a+","+b+","+c+",'"+result+"')");
               statement2    = dbConnection.createStatement();
              }
            c-=stepC;
            }
            b-=stepB;
            c=maxC;
        }
        a-=stepA;
        b=maxB;
        c=maxC;
    } 
}
catch(Exception e){System.out.println("Error while calc"+e.getMessage());} 
return count;
}

What this does is some calculations depending on a,b and c and stores the result to the database. all is working fine, except the speed of it. with that simple timing i see some really strange results!
On a single run i get these times:
seconds count Calcs per sec seconds for 1 calc
68 8000 117.647058823529 0.0085
154 64000 415.584415584416 0.00240625
1516 64000 42.2163588390501 0.0236875
3693 64000 17.3300839425941 0.057703125
That means that the first time around the program was calculating 117 calculations in a second and 4 times later it get 10 times slower!

but if i stop the process and re run it i get

seconds count Calcs per sec seconds for 1 calc
1576 64000 40.6091370558376 0.024625

which is faster than the later example, but still slower than the first

Do you have any idea what i could do to make that faster?

thanks
ilias

[Tolls]

You cannot time Java like that.
It's simply not meaningful.
You have no idea if there is some other contention going on with the OS for example.
As well as the JVM compiling sections of the code as it deems necessary.

So, in order to get sensible timings, you need to run it a several times to warm up the JVM, then run the code several hundred times, timing the whole lot (not individual runs), and calculate the average.

[skarosg3]

you need to run it a several times to warm up the JVM, then run the code several hundred times, timing the whole lot (not individual runs), and calculate the average.

What do you mean warm up the jvm.
this method is been called few times, but as i (not scientifically) said, each time the function is called, it is slower and slower.

[Tolls]

The JVM (HotSpot) does not always compile every bit of code into native code.
It only does that for code which has been accessed quite alot.
Not sure how many times a method needs to be called, but it's hundreds (possibly over a thousand).
This compilation slows the system down, so you could be (not saying it's the case) seeing bits of the code being compiled as you're running.

[skarosg3]

in other words,
if i call a method 10 times, and within that method i have a loop of 10 times, it is better (or might be) to call the method once and have a loop of 100 times there?
it does make sense somehow.

[Tolls]

Don't write code based on this, for gods sake!
For all you know the underlying implementation may change...never mind that the compiler may do useful stuff with it all anyway (the old "write dumb code").

Write clear code. If that first loop of 10 represents something different to the second loop of 10 then they are not, in fact, the same loop.

I'm only mentioning the "warming up" when it comes to timing algorithms.
You need to be aware what it is you are timing.

[skarosg3]

well, i only mention that because on my case all 10 calls to the method do the same thing, with different data, ofcourse.
Actually i am giving that a try to see how it works. I have some issues though, but i will manage.
thanks

[Tolls]

I will say that that one method you have up there should be broken down into smaller units.
It's a monster.

The query should be one part, ensuring you close the statement and resultset etc (those are resources, and need to be closed properly).

And now that I look at that code, I didn't notice you were doing db work inside that method you're timing. There's a load more timing variables with that...

Since I don't see an rs.close() or statement.close() anywhere in there I am wondering if your db is leaking resources because of this.

[skarosg3]

I was thinking about time consumsions from the db, so now i am trying to change it in order to have a arraylist, so i will call the db once.
i was sure that i closed everything. will have a look.

[Tolls]

Taking that code up there:

try{
// OPEN STUFF HERE
    statement    = dbConnection.createStatement();
    rs=statement.executeQuery("select * from engg where ht like '%"+HTeam+"%' and at like '%"+ATeam+"%'");

...

    while ((a>minA)){
...
        while((b>minB)){
...
            while (c>minC){count++;
            if(result.equalsIgnoreCase(myResult))
            {  
               statement.executeUpdate("insert into table3 values("+a+","+b+","+c+",'"+result+"')");
// CREATE ANOTHER STATEMENT (not needed by the looks of it)
               statement2    = dbConnection.createStatement();
              }
}
catch(Exception e){System.out.println("Error while calc"+e.getMessage());} 
// NOTHING CLOSED BY HERE.

That exception catch should print the stack trace.
The result set should be closed in a finally block.

The query should probably be a PreparedStatement, which you can keep open until you're about to close the connection.
The Update should also be a PreparedStatement, which can similarly be kept open.
The updates can, in fact, be batched, which will improve performance as it will mean only a single call to the db.

[skarosg3]

Actually, i am working on that right now, but it seems that i messed something up and now it is not working(error: This statement has been closed). i will figure it out thought.

What do you mean preparedStatement? i dont think i have heard that before in my life. will have a look at that when i find my error.

[Tolls]

PreparedStatement.

When you're doing SQL that involves a query that takes variable parameters (ie the SQL is not static) then it is advisable to use a prepared statement. First it prevents SQL injection. Second it also means not having to escape special characters, or worry about quotes. Third, if used as part of a loop, it allows the db to pre-compile the query, generally making it faster (with queries it also allows the db to skip building a query plan, since it already has one).

Also, don't forget batching.

[skarosg3]

First of all let me say that this is a personal project. Its not a paid job, so this is a reason that i might not have things through.
Also, this project grew up enormous in time. when i started it i never thought that it would be so big.
And thirdly, apparently my db skills are not as high as i thought. i didnt know that you can prepare the sql commands in advanced, except if you mean something like
String Command="select * from thatTable";
but this would make no difference.
I will look on these matters and see what i can do.
Thanks for your hints and especially on the close(). i forgot to close quite alot of them.

[travishein]

If it helps, here is your code sample refitted to use prepared statements, and some reorganizing of the nested query into its own method, so as to demonstrate how to handle invoking nested queries and closing the resources as you go.

/**
 * A Java bean to model the results from the engg table.
 * Reading the result set from a query into a bean might make things faster, partcularly if there are many columns and many cases 
 * where you would need to do rs.getString("aColumn"), because invoking the rs.getX() a bunch of times is not always as fast (or 
 * depending on the cursor model, even possible), as reading rs.getX() into a local variable, and then invoking the getX() on that 
 * local variable.
 */
class EnggRecord {
  String ht;
  int hs;
  String at;
  int as;

  public void EnggRecord(ResultSet rs) {  
    this.ht = rs.getString("HT");
    this.hs = rs.getInt("hs");
    this.at = rs.getString("AT");
    this.as = rs.getInt("as");
  } 
  
  public String getResult() {
    String result = ht + hs + at + as;
     if (this.hs >this.as) {
            result="1";
     }       
     else if (this.hs <this.as) {
            result="2";
     }
     // if not > and not < then must be == 
     //if ((rs.getInt("hs"))==(rs.getInt("as")))
     else {
            result="3";
     }
  }
  
  public String getHt(){
    return ths.ht; 
  }
  
  public int getHs() {
    return this.hs;
  }
  
  public String getAt() {
    return this.at;
  }  
    
  public int getAs() {
    return this.as;
  }
}



public  int calcOd(String HTeam,int HW,int HD,int HL,String ATeam, int AW,int AD,int AL){

// I like to declare variables 1:1,  makes it easier to read and maintain I think.
int HT=HW+HD+HL;
int AT=AW+AD+AL;
int AG;
int HG;
int newGames=0;
float HWR=(HW*100/HT);
float AWR=(AW*100/HT);
float HLR=(HL*100/HT);
float ALR=(AL*100/AT);
float Home=0,
float Away=0;
float maxA=Float.parseFloat(MaxAText.getText());
float maxB=Float.parseFloat(MaxAText.getText());
float maxC=Float.parseFloat(MaxC.getText());
float minA=Float.parseFloat(MinA.getText());
float minB=Float.parseFloat(MinA.getText());
float minC=Float.parseFloat(MinC.getText());
float stepA=Float.parseFloat(StepA.getText());
float stepB=Float.parseFloat(StepA.getText()); 
float stepC=Float.parseFloat(StepC.getText());
float a=maxA;
float b=maxB;
float c=maxC; 
String myResult="";
boolean done=false;
int count=0;

String[] games=new  String[5];

Connection dbConnection; // I assume you are using a JDBC connection pool ?

// example use of prepared statements.
PreparedStatement statement = null;
ResultSet rs = null;

try{

    statement    = dbConnection.prepareStatement("select * from engg where ht like ? and at like ?");
    statement.setString(1, "%"+HTeam+"%");
    statement.setString(2, "%"+ATeam+"%");
    rs=statement.executeQuery();
    
    // Note: you seem to be only carrying about the first result found.
    // if the SQL query above might ever return more than one row?
    if (rs.next() ) {
  
      EnggRecord record = new EnggRecord(rs);
      String result= record.getResult();
      
     
      games[0]=result.getHt();
      games[1]=result.getHs();
      games[2]=result.getAt();
      games[3]=result.getAs();
      
    } // if has a result set.
    
    while ((a>minA)){
        Home=a*HWR;
        while((b>minB)){
            Away=b*AWR;
            while (c>minC){
            count++;
             if(Home-Away>c) {
                    myResult="1";
             }
             else if(Home-Away<-1*c) {
                myResult="2";
                if((Home-Away>=-1*c)&&(Home-Away<=c)) {
                    myResult="3";
                    if(HLR-ALR>0.8) {
                      myResult="2";
                    }
                }
                if(result.equalsIgnoreCase(myResult)) {  
                  // perform the update using a function
                  //statement.executeUpdate("insert into table3 values("+a+","+b+","+c+",'"+result+"')");
                  doInsertTable3(dbConnection, a, b, c, result);
                  // note, instead of invoking doInertTable3 
                  
                  // why is we do this, it's not used anywhere here?
                  //statement2    = dbConnection.createStatement();
                }
              c-=stepC;
            }
            b-=stepB;
            c=maxC;
        }
        a-=stepA;
        b=maxB;
        c=maxC;
    } //while
  } // try
  catch(Exception e) {
    System.out.println("Error while calc"+e.getMessage());
  } // catch
  finally {
    // now close resources
    if (rs != null) {
      try {
        rs.close();
      }
      catch (SQLException ex) {
        // ignore
      }
      rs = null;
    }
    if (statement != null) {
      try {
        statement.close();
      }
      catch (SQLException ex) {
        // ignore
      }
      statement = null;
    }
    
    // i assume if you are using a jdbc connection pool, we need to close the connection too.
    if (dbConnection != null) {
      try {
        dbConnection.close();
      }
      catch (SQLException ex) {
        // ignore
      }
      dbConnection = null;
    }
 } // finally
 
  return count;

} // method
 
public void doInsertTable3(Connection con, float a, float b, float c, String result) throws SQLException {
  PreparedStatement statement = null;
  try {
    statement = con.prepareStatement("insert into table3 values(?, ?, ?, ?)");
    statement.setFloat(1, a);
    statement.setFloat(2, b);
    statement.setFloat(3, c);
    statement.setString(4, result);
    statement.executeUpdate();
  }
  // no need for a catch here, exception will be thrown to caller method
  finally {
    if (statement != null) {
      try {
        statement.close();
      }
      catch (SQLException ex) {
        // ignore
      }
      statement = null;
    }
  }
}

Another idea, depending on if it is possible in your situation, if you are doing a main loop over some data and wanting to perform insert or update operations for stuff at the same time, if those updates or inserts aren't transactional (not related), it might be possible to separate the for each... part from the insert/update part by creating a queue (e.g. Vector of beans, where the for() loop puts request for stuff to be written into the queue, and a separate thread reads the vector queue and for each item peforms a database insert (using its own separate connection object from the jdbc connection pool). this kind of threaded database operations allow for the insert / updates to make their way into the database without causing the main loop thread to block. Though the implementaion largely depends on the data being operated on.

[skarosg3]

Thanks for you hints travishein, but to be honest, i cant understand why a prepared statement might be better or faster than a normal one.
I was thinking that maybe the long delay might be due to the multiple inserts that i do, so i tried to remove the insert inside the loop, and i used a string variable to store the values that i want to insert like that:
String insert=insert+"("+a+","+b....
and then call updatequery once.
Well it seems to me that this took much longer! i was surprised to be honest.

About my code, yes, i only called that method with a pair of data, so the rs would only return one line. i know and i am sure about it.
Actually, i changed that, but again there was not any difference in process time.

I came to the conclusion, that whatever i do, if i want to keep the nested while loop as it is, it is going to be a long process for big numbers. i am having around 64000 repetitions, so there is no way to get it any faster.

I was thinking about threads as well, mainly because while in this process the program is not responding, but on the other hand, the user can not do anything before these data are been processed. they are crucial for the rest of the application. so i decided against it, since i would have to change a big amount of code, with no apparent gain.

on your comments here:

finally {
    // now close resources
    if (rs != null) {
      try {
        rs.close();
            }
      catch (SQLException ex) {
        // ignore
      }
      rs = null;
    }
    if (statement != null) {
      try {
        statement.close();
      }
      catch (SQLException ex) {
        // ignore
      }
      statement = null;
    }
    
    // i assume if you are using a jdbc connection pool, we need to close the connection too.
    if (dbConnection != null) {
      try {
        dbConnection.close();
      }
      catch (SQLException ex) {
        // ignore
      }
      dbConnection = null;
    }
 } // finally

you dont really close the rs and statement, do you. you set them to null. also i do you a jdbc connection, but i need it for the rest of the program(this is only a small portion, so i dont close it till the end.

thanks, though. i will look at your tips and see if i can use them.
ilias

[Tolls]

I listed the reasons for using a PreparedStatement...performance is low on the list, but it is there.

This is where knowing how most DBs work comes in handy.

They have to interpret the statement and then come up with an execution plan (this applies mainly to queries and updates, but also to a lesser extent inserts (especially where keys and unique values are concerned)).

The db will re-use an execution plan if it finds one in its pool of "recent" SQL statements. It does this search based purely on the SQL string supplied. So these two statements will result in two entries in that pool:

SELECT * FROM my_table WHERE some_column = 1;
SELECT * FROM my_table WHERE some_column = 2;

because the strings are different, meaning that even though you ran the first one and that they will undoubtedly (with exceptions) result in the same plan, the db will still try and come up with a new plan for the second one.

This prepared statement, however, will result in a single plan allowing the db to completely skip the execution plan step:

SELECT * FROM my_table WHERE some_column = ?;

Now, even better, for the insert you ave, would be to declare the PreparedStatement outside of the loop in which it is used. This allows JDBC to do some jiggery pokery and skip some steps itself. And if you were to throw in batching, which allows (as I said above) a single call to the db with all your inserts, and you might well see some marked improvement (assuming you time this correctly).

So, to alter the above slightly:

    insertStatement = con.prepareStatement("insert into table3 values(?, ?, ?, ?)");

    while ((a>minA)){
...
        while((b>minB)){
...
                if(result.equalsIgnoreCase(myResult)) {  
                  doInsertTable3(insertStatement, a, b, c, result);
                }
...
    } //while
    // Finally execute our batch of inserts.
    insertStatement.executeBatch();
...

...

public void doInsertTable3(PreparedStatement statement, float a, float b, float c, String result) throws SQLException {
    statement.setFloat(1, a);
    statement.setFloat(2, b);
    statement.setFloat(3, c);
    statement.setString(4, result);
    statement.addBatch();
}