7
\$\begingroup\$

I've written a bit of code designed to go through a log file and determine successes and failures for certain criteria for each server in an environment. The code works just fine, except that on environments of larger size, it's just too slow. Currently it's running on a 2000 server environment with 18 criteria and 5000 lines of logs being checked and takes about 14 minutes on my machine. The problem is that it's intended to be used on an environment of about 18000 servers with about 40 criteria and 175,000 lines of logs.

I'm hoping someone can help me come up with something I'm not seeing right now that would help with the speed, because waiting an hour and hoping Excel doesn't crash doesn't really speed up this process.

Here's what I'm trying to do:

Sample Log File

Sample Final

This is the code I'm running (there are comments in there to help understanding):

Dim lastCol As Double, lastRow As Double, lastSensorRow As Double
Sub B_L3ToolRun()
    SpeedUp
    Dim numSuc As Long, numMaj As Long, numWar As Long, numCri As Long, numErr As Long
    lastRow = Sheets("Results").Range("A" & Rows.Count).End(xlUp).Row //End of IPs
    lastSensorRow = Sheets("Data").Range("A" & Rows.Count).End(xlUp).Row //End of Log File
    lastCol = 3                                     //Minimum columns used
    For I = 4 To 150                                //Discovers number of criteria used
        If Cells(1, I).Value <> "" Then
            lastCol = lastCol + 1
        End If
    Next I
    Sheets("Results").Activate
    For J = 3 To lastCol //This is a check for each criteria being checked
        For I = 2 To lastRow //This is a check for each IP in the list (2000)
            numSuc = WorksheetFunction.CountIfs(Sheets("Data").Range("B:B"), "*" & Sheets("Results").Cells(1, J).Value & "*", _   //This checks for number of given criteria
                                                Sheets("Data").Range("E:E"), "=normal", _                                         //This refines it to only successful ones
                                                Sheets("Data").Range("C:C"), "*" & Sheets("Results").Cells(I, 1).Value & "*")     //This limits that to number of successful on that IP

            numMaj = WorksheetFunction.CountIfs(Sheets("Data").Range("B:B"), "*" & Sheets("Results").Cells(1, J).Value & "*", _
                                                Sheets("Data").Range("E:E"), "=major", _                                          //Same as above but "major" error
                                                Sheets("Data").Range("C:C"), "*" & Sheets("Results").Cells(I, 1).Value & "*")

            numWar = WorksheetFunction.CountIfs(Sheets("Data").Range("B:B"), "*" & Sheets("Results").Cells(1, J).Value & "*", _
                                                Sheets("Data").Range("E:E"), "=warning", _                                        //Same as above but "warning" error
                                                Sheets("Data").Range("C:C"), "*" & Sheets("Results").Cells(I, 1).Value & "*")

            numCri = WorksheetFunction.CountIfs(Sheets("Data").Range("B:B"), "*" & Sheets("Results").Cells(1, J).Value & "*", _
                                                Sheets("Data").Range("E:E"), "=critical", _                                       ////Same as above but "critical" error
                                                Sheets("Data").Range("C:C"), "*" & Sheets("Results").Cells(I, 1).Value & "*")
            numErr = numMaj + numWar + numCri
            If numErr > 0 Or numSuc > 0 Then
                Sheets("Results").Cells(I, J).Value = numSuc & "/" & numErr                   //I want them displayed  in this format "count success/count fail"
            End If
        Next I
    Next J
    SlowDown
End Sub
Private Sub SpeedUp()
    Application.ScreenUpdating = False
    Application.DisplayStatusBar = False
    Application.Calculation = xlCalculationManual
    Application.EnableEvents = False
    ActiveSheet.DisplayPageBreaks = False
    Application.DisplayAlerts = False
End Sub
Private Sub SlowDown()
    Application.ScreenUpdating = True
    Application.DisplayStatusBar = True
    Application.Calculation = xlCalculationAutomatic
    Application.EnableEvents = True
    ActiveSheet.DisplayPageBreaks = displayPageBreaksState
    Application.DisplayAlerts = True
End Sub
\$\endgroup\$
  • \$\begingroup\$ Cross-posted on SO. \$\endgroup\$ – Mast Nov 25 '15 at 10:37
  • \$\begingroup\$ Why not just use a formula for this? As it is, I don't see why there's any code necessary. \$\endgroup\$ – RubberDuck Nov 25 '15 at 13:23
7
\$\begingroup\$

Things I like:

  • You've used meaningful variable names (I think numMajorErr would be an even better convention for your counters but it's a minor quibble)

  • Your code is nicely indented, good use of _ to split your functions onto multiple lines for readability.

  • Your code is nicely commented. This combined with the above 2 points makes it very easy to understand and follow.

And now for the review:


Tips and Tricks

Codenames

Sheet name/Codename in project window

All sheets have a "Name" which is what the user sees and can edit. Sheets("Data") is referencing a sheet name. However, names can change, especially when users are involved. Codenames, on the other hand, can only be set/changed in the VBE:

Sheet codename in property window

Assuming your "Data" sheet has codename "ws_Data", these 2 statements are equivalent:
Sheets("Data").Range(), ws_Data.Range. Always reference codenames and not only can you give your sheet variables meaningful names, there's no danger of someone changing the name and breaking your macro.

Final Row/Column

Personally, I recommend

finalRow = Cells(Rows.Count, 1).End(xlUp).Row
finalColumn = Cells(1, Columns.Count).End(xlToLeft).Column

No strings, no loops, just neat, clean and simple.


Proper Variable Scoping

Variables should be declared as close as is practicable to where they're used. Crucially, Variables should be Procedure-Level where possible, then Module-Level where possible, then and only then Global level.

Dim lastCol As Double, lastRow As Double, lastSensorRow As Double

Why on earth are these Module-Level variables? There's only one procedure. Develop good habits now and put them inside their procedure where they belong. As a side note, if you only ever use Dim for Procedure-Level, Private for Module-Level and Public for Global-Level variables, it will make reviewing your code (especially on larger projects) even easier.


And now for the performance optimisation

You have 2 options:

  1. Just put your worksheet functions in an actual worksheet.

Create a new sheet, fill in a countifs() that references the other sheet and auto-fill a grid as appropriate. Excel is highly optimised for large grids of worksheet functions.

  1. Which I will focus on here: Store all the sheet data in arrays and work from those.

Accessing a worksheet is slow. And you are accessing it every time you specify a cell reference. An array, on the other hand, is just data laid out in sequential memory blocks, so accessing a location in an array is orders of magnitude faster than accessing a location in a worksheet.

Start your program doing this:

Dim wsResultsTable As Variant
    wsResultsTable = Array()

Dim wsDataTable As Variant
    wsDataTable = Array()

For each worksheet:

'/ Determine Range of worksheet Data
'/ <Range> = ws.Range()
'/ <Array> = <Range>
'/ Now Array(1,1) = Cells(1,1).value etc.

You're going to be referencing the same 3 columns over and over again, so store them in their own arrays:

Dim bColumnData As Variant
    bColumnData = Array()

Dim colIndex As Long
    colIndex = 2 ' "B" Column

    Dim LB1 As Long, UB1 As Long ' Determine L/U bounds of the array
    LB1 = LBound(wsDataTable, 1)
    UB1 = UBound(wsDataTable, 1)

    ReDim bColumnData(LB1 To UB1) ' Create 1-D array of the same size

    For i = LB1 To UB1
        bColumnData(i) = wsDataTable(i, colIndex)
    Next i

Repeat for the other 2 columns, and you now have 3 lists of column data stored in arrays.

Now just replace your countIfs with a VBA loop:

for a
    for b
        for i = LB1 to UB1
            if <criteria 1> then <counter 1> = <counter 1> + 1
            if <criteria 2> then <counter 2> = <counter 2> + 1
            if <criteria 3> then <counter 3> = <counter 3> + 1
            if <criteria 4> then <counter 4> = <counter 4> + 1
        next i

        '/ Print results to sheet etc.
    next b
next a

And this should run at least an order of magnitude faster.


Optimisation Summary

If you're working with data, then strip away all of the excess (containers, sheets, formatting, highlighting etc.) and put the raw data in an Array.

If you can (easily) re-create a worksheet function in VBA, then do so.

If you're going to be referencing the same things over and over, store them in a variable and reference the variable. The same goes for the results of calculations (A.K.A. memoisation).

Get really familiar with using Arrays and moving data from sheets to arrays and back again. It'll save you a lot of trouble down the road.

\$\endgroup\$
  • \$\begingroup\$ Wow. This was great. I'll definitely all of this to heart as I try to optimize it. (especially code names for future projects) If I can get familiar enough with it, I might be able to apply it to other tools we use as well. You said arrays is orders of magnitude faster, any idea how much quicker I'd be looking at? \$\endgroup\$ – Mike Nov 30 '15 at 13:47
  • \$\begingroup\$ Dependng on circumstances: 10 - 100 times faster \$\endgroup\$ – Kaz Nov 30 '15 at 14:07
0
\$\begingroup\$

For loop to find last column. If the first row is header I recommend to exit loop when blank or empty cell and not need to increment of lastCol. You can add I value to lastCol when found empty cell like this.

For I = 4 To 150   //Discovers number of criteria used
    If Cells(1, I).Value = "" Then
        lastCol = I
        exit for
    End If
Next I
\$\endgroup\$
  • 1
    \$\begingroup\$ And what if there is no blank cell before I reaches 150? lastCol will be undefined or zero then, instead of 150. \$\endgroup\$ – user1016274 Nov 27 '15 at 12:12
  • \$\begingroup\$ @user1016274 following mine it hadn't defined. The question owner defined lastCol before enter loop. That is a reason I didn't . \$\endgroup\$ – Monkawee Maneewalaya Nov 30 '15 at 1:16
  • \$\begingroup\$ Brief explanation in response to user1016274: There are at most (in the current version of the application) 115 criteria, and more often than not, it will be around 30-40. There should never not be a blank before 150. The only reason I'm even doing it this way is because I've been having issues with the traditional method of grabbing the lastCol giving me the wrong value This looks like a much cleaner solution though, much appreciated. \$\endgroup\$ – Mike Nov 30 '15 at 13:57

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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