Why code is maxing out the CPU while querying the database?

Question

My C# code below checks a SQL database to see if a record matches a ClientID and a User Name. If more than 15 or more matching records are found that match, the CPU on my Windows 2008 server peaks at about 78% while the 15 records are found while the below C# code executes. The SQL Server 2008 database and software is located on another server so the problem is not with SQL Server spiking the CPU. The problem is with my C# software that is executing the code below. I can see my software executable that contains the C# code below spike to 78% while the database query is executed and the records are found.

Can someone please tell me if there is something wrong with my code that is causing the CPU to spike when 15 or more matching records are found? Can you also please tell/show me how to optimize my code?

Update: If it finds 10 records, the CPU only spikes at 2-3 percent. It is only when it finds 15 or more records does the CPU spike at 78% for two to three seconds.

//ClientID[0] will contain a ClientID of 10 characters
//output[0] will contain a User Name
char[] trimChars = { ' ' };
using (var connection = new SqlConnection(string.Format(GlobalClass.SQLConnectionString, "History")))
{
    connection.Open();
    using (var command = new SqlCommand())
    {
        command.CommandText = string.Format(@"SELECT Count(*) FROM Filelist WHERE [ToAccountName] = '" + output[0] + @"'");
        command.Connection = connection;
        var rows = (int) command.ExecuteScalar();
        if (rows >= 0)
        {
            command.CommandText = string.Format(@"SELECT * FROM Filelist WHERE [ToAccountName] = '" + output[0] + @"'");
            using (SqlDataReader reader = command.ExecuteReader())
            {
                if (reader.HasRows)
                {
                    while (reader.Read())
                    {
                        //Make sure ClientID does NOT exist in the ClientID field
                        if (reader["ClientID"].ToString().TrimEnd(trimChars).IndexOf(ClientID[0]) !=
                            -1)
                        {
                            //If we are here, then do something
                        }
                    }
                }
                reader.Close();
                reader.Dispose();
            }
        }
        // Close the connection
        if (connection != null)
        {
            connection.Close();
        }
    }
}

Prepare for the hackers: lots of scope for SQL injection. Or, better, use parametrised queries. — Richard
– Richard, Commented Nov 17, 2012 at 16:59
This code runs on an internal system and it is not exposed to the outside internet. — fraXis
– fraXis, Commented Nov 17, 2012 at 17:00
For first,"using" is incapsulates disposal of disposable objects.Then you bring all data to client then checks them for ClientID. Why not SELECT * FROM Filelist WHERE [ToAccountName] = @ToAccountName AND ClientID = @ClientID ? — Hamlet Hakobyan
– Hamlet Hakobyan, Commented Nov 17, 2012 at 17:01
Additionally: using blocks clean up resources, you don't need to as well (ie. your Connection.Dispose is duplicating the using block). — Richard
– Richard, Commented Nov 17, 2012 at 17:01
@fraXis Most security breaches come from insiders. It is trivially easy to use parametrised queries, and safe. Just always use them: default to good practice. Additionally they can be quicker as well (the server can cache the query plan). — Richard
– Richard, Commented Nov 17, 2012 at 17:03

abatishchev · Accepted Answer · 2012-11-17 17:34:54Z

4

You can decrease the number of database access from 2 to 1 if will remove first query, it is not necessary.

using (SqlConnection connection = new SqlConnection(connectionString))
using (SqlCommand command = connection.CreateCommand())
{
    command.CommandText = "SELECT ClientID FROM dbo.Filelist WHERE ToAccountName = @param"; // note single column in select clause
    command.Parameters.AddWithValue("@param", output[0]); // note parameterized query

    connection.Open();
    using (SqlDataReader reader = command.ExecuteReader())
    {  
        while (reader.Read()) // reader.HasRow is doubtfully necessary
        {
            // logic goes here
            // but it's better to perform it on data layer too

            // or return all clients first, then perform client-side logic
            yield return reader.GetString(0);
        }
    } // note that using block calls Dispose()/Close() automatically
}

edited Nov 17, 2012 at 17:34

answered Nov 17, 2012 at 17:13

abatishchev

101k88 gold badges304 silver badges443 bronze badges

Sign up to request clarification or add additional context in comments.

2 Comments

Nick.Mc Over a year ago

This code means you are not performing string comparisons in your C# code and you will be retrieving less (possibly a lot less) rows locally. Neither of which explain the magic number '15', but the more performance bottlenecks you fix, the easier it will be to find the root cause.

abatishchev Over a year ago

@ElectricLlama: I fully support application server side calculations and or better to say business logic, and usually go this way. But don't be surprised that app server will require more (and more) resources. And from careful OP reading I found that this is his issue! Not DB server CPU usage spike but app server on DB request! (see his comments to the question itself)

Nesim Razon · Accepted Answer · 2012-11-17 17:11:17Z

3

Change this:

SELECT * FROM Filelist

To this:

SELECT ClientID FROM Filelist

And check for performance. I suspect there is a blob field on your select. Also select * is not recommended, write your exact interested fields in your query.

answered Nov 17, 2012 at 17:11

Nesim Razon

9,8043 gold badges38 silver badges48 bronze badges

1 Comment

Philip Daniels Over a year ago

I suspect you're right, I would definitely look at this if I was the fraXis. Unfortunately we don't know what is happening in the "do something" block :-)

Philip Daniels · Accepted Answer · 2012-11-17 17:23:42Z

3

Nothing looks obviously CPU intensive, but one problem does stand out.

You are running a query to count how many records there are

"SELECT Count(*) FROM Filelist WHERE [ToAccountName] = '" + output[0] + @"'"

Then, if more than 0 is returned, you are running another query to get the data.

"SELECT * FROM Filelist WHERE [ToAccountName] = '" + output[0] + @"'"

This is redundant. Get rid of the first query, and just use the second one, checking to see if the reader has data. You can also get rid of the HasRows call and just do

using (SqlDataReader reader = command.ExecuteReader())
{
    while (reader.Read())
    {
    }
}

answered Nov 17, 2012 at 17:23

Philip Daniels

1,0441 gold badge9 silver badges26 bronze badges

Comments

abatishchev · Accepted Answer · 2012-11-17 17:15:37Z

1

Please consider what already said about parametrized queries.

Beside that, I think that the only big issue could arise in the following block:

while (reader.Read())
{
    //Make sure ClientID does NOT exist in the ClientID field
    if (reader["ClientID"].ToString().TrimEnd(trimChars).IndexOf(ClientID[0]) != -1)
    {
        //If we are here, then do something
    }
}

So try to just cache your reader.Read() data in some local variable, releasing the SQL resources asap, then you can work on the data you just retrieved. Eg:

List<string> myRows = new List<string>();
while (reader.Read())
{
   myRows.Add(reader["ClientID"].ToString();
}
/// quit the using clause
/// now elaborate what you got in myRows

edited Nov 17, 2012 at 17:15

abatishchev

101k88 gold badges304 silver badges443 bronze badges

answered Nov 17, 2012 at 17:12

LittleSweetSeas

7,1242 gold badges24 silver badges27 bronze badges

Comments

Richard · Accepted Answer · 2012-11-17 17:19:02Z

0

There is nothing in the code to indicate a performance problem.

What does SQL Profiler show?

(Both in terms of query plan, and server resources used.)

Edit: To make this clearer: you have one measurement that might indicate an issue. You now need to measure more deeply to understand if it really is a problem, only you can do this (no one else has access to the hardware).

edited Nov 17, 2012 at 17:19

answered Nov 17, 2012 at 17:05

Richard

110k21 gold badges214 silver badges279 bronze badges

8 Comments

fraXis Over a year ago

Would I run SQL Profiler on both the server running the C# code and the server running SQL Server 2008, or just the server running SQL Server 2008?

fraXis Over a year ago

Would a 78 CPU spike be considered a performance problem though for finding such a small amount of records?

Richard Over a year ago

@fraXis 78% would be an issue on a 256 core monster server with matching UI and memory. On a single Atom core with limited RAM and the results not cached it may be normal.

abatishchev Over a year ago

Even on monster it depends. 78% is normal too if you scan 100TB database.

fraXis Over a year ago

The database is very small. Maybe only about 300 records total.

|

Richard · Accepted Answer · 2012-11-19 15:58:53Z

0

I strongly recommend that you get a copy of dotTrace from JetBrains.

At the very least, profiling the client code will help you identify/eliminate the source of the CPU spike.

edited Nov 19, 2012 at 15:58

Richard

110k21 gold badges214 silver badges279 bronze badges

answered Nov 17, 2012 at 17:25

code4life

15.8k8 gold badges56 silver badges82 bronze badges

1 Comment

code4life Over a year ago

@Richard: That's not what the OP said. The OP clearly states that the issue is with the desktop.

BenMorel · Accepted Answer · 2013-10-31 20:22:58Z

0

I recommend using parameters as suggested, however, I have seen performance problems where the type of the string column does not match the C# string. In these cases, I suggest specifying the type explicitly.

Like this:

command.CommandText = "SELECT ClientID FROM dbo.Filelist WHERE ToAccountName = @accountName"; 
command.Parameters.Add("@accountName", SqlDbType.NVarChar, 16, output[0]);

Or this:

SqlParameter param = command.Parameters.Add(
    "@accountName", SqlDbType.NVarChar);
param.Size = 16; //optional
param.Value = output[0];

edited Oct 31, 2013 at 20:22

BenMorel

37k52 gold badges208 silver badges339 bronze badges

answered Nov 17, 2012 at 18:03

weston

55k23 gold badges151 silver badges209 bronze badges

Collectives™ on Stack Overflow

Why code is maxing out the CPU while querying the database?

7 Answers 7

2 Comments

1 Comment

Comments

Comments

8 Comments

1 Comment

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

7 Answers 7

2 Comments

1 Comment

Comments

Comments

8 Comments

1 Comment

Comments

Your Answer

Sign up or log in

Post as a guest

Related