Why is my excel macro so slow?

I am just getting into vba scripting in Excel 2007, and I'm just mucking around to see if its useful.

So I created this basic script for filling a bunch of cells with values. And at the same time it loops through each worksheet. I have approx 10 worksheets in the excel document.

The script runs takes about 2,5 seconds to run...!?!?

NOTE: I know that the script is not really accomplishing anything useful. Thats not the point here. The point is that I am staggered that it takes 2,5 seconds to do such a basic operation!?

So my question: Is this normal?
I have a pretty new computer with plenty of computer power. I would expect this script to run in the blink of an eye?
Am I missing something?

Thanks

Sub Test()
     
    Dim currSumRow As Integer
    Dim currSumCol As Integer
    
    'loop each column in turn from left to right >>>

    For currSumCol = 5 To 10
        
        'nested: loop each row from top to bottom
            For currSumRow = 5 To 10
                
                'nested: loop each sheet from first to last
                    For Each sht In Sheets
                          'put the string testing into each cell on the worksheet "test". Just for fun :) Worksheets("test").Cells(currSumRow, currSumCol).Value = "testing"
                    Next sht

            Next currSumRow 'end row loop

    Next currSumCol 'end column loop
    
End Sub

Open in new window

LVL 5
NiklasMollerAsked:
Who is Participating?
I wear a lot of hats...

"The solutions and answers provided on Experts Exchange have been extremely helpful to me over the last few years. I wear a lot of hats - Developer, Database Administrator, Help Desk, etc., so I know a lot of things but not a lot about one thing. Experts Exchange gives me answers from people who do know a lot about one thing, in a easy to use platform." -Todd S.

jppintoCommented:
I've tested your code on my computer and it tooked 00:00:01 to run, with 10 sheets also! I can't see what can be the problem with your computer but it isn't the code!

jppinto
0
patrickabCommented:
NiklasMoller,

As below it is almost instantaneous.

Patrick
Sub Test()
Dim sht As Worksheet
Dim currSumRow As Integer
Dim currSumCol As Integer

'loop each column in turn from left to right >>>

For currSumCol = 5 To 10
    'nested: loop each row from top to bottom
    For currSumRow = 5 To 10
        'nested: loop each sheet from first to last
        For Each sht In Worksheets
            sht.Cells(currSumRow, currSumCol) = "testing"
        Next sht
    Next currSumRow 'end row loop
Next currSumCol 'end column loop
    
End Sub

Open in new window

0
Brian BEE Topic Advisor, Independant Technology ProfessionalCommented:
What is your available memory, OS and drive space? The problem could be there.
0
Cloud Class® Course: SQL Server Core 2016

This course will introduce you to SQL Server Core 2016, as well as teach you about SSMS, data tools, installation, server configuration, using Management Studio, and writing and executing queries.

DonkeyOteCommented:
Do you have Volatile calculations in your sheets ?

Iteration is generally a slow method - disabling screen repaint etc will also improve code.

For a good overview of basics in terms of code optimisation:

http://blogs.msdn.com/excel/archive/2009/03/12/excel-vba-performance-coding-best-practices.aspx
0

Experts Exchange Solution brought to you by

Your issues matter to us.

Facing a tech roadblock? Get the help and guidance you need from experienced professionals who care. Ask your question anytime, anywhere, with no hassle.

Start your 7-day free trial
jppintoCommented:
If I comment this part of the code is also instantaneous:

'put the string testing into each cell on the worksheet "test". Just for fun :) Worksheets("test").Cells(currSumRow, currSumCol).Value = "testing"

Below you can see the code that I've tested.

jppinto
Sub Test()
      Debug.Print (Now())
    Dim currSumRow As Integer
    Dim currSumCol As Integer
     
    'loop each column in turn from left to right >>>
 
    For currSumCol = 5 To 10
         
        'nested: loop each row from top to bottom
            For currSumRow = 5 To 10
                 
                'nested: loop each sheet from first to last
                    For Each sht In Sheets
                        sht.Cells(currSumRow, currSumCol).Value = "testing"
                    Next sht
 
            Next currSumRow 'end row loop
 
    Next currSumCol 'end column loop

    Debug.Print (Now())
End Sub

Open in new window

0
patrickabCommented:
ps. It is not logical to specify in your code that you place "Testing" in each worksheet and then restrict it to a worksheet that is named 'test'. If you want to do that then don't both cycling through all the worksheets and just specify the 'test' worksheet.

Patrick
0
patrickabCommented:
NiklasMoller,

For such a small macro code optimisation is irrelevant. The whole macro executes within less than a second so it really doesn't matter.

Equally unless you have a ton of formulae with volatile functions in them, they will have little or no effect on the macro execution time.

Finally for such a small macro it is unnecessary to disable ScreenUpdating. For a larger macro and/or with far more data being updated it becomes relevant but for your demo-macro it is irrelevant.

Patrick
0
DonkeyOteCommented:
quote-Patrick
Equally unless you have a ton of formulae with volatile functions in them, they will have little or no effect on the macro execution time.

Quite, unless, do not discount possible causes based upon your own assumptions which may well be misguided.

Rather than discounting all other posts why not elaborate as to what you believe to be potential causes of the slow down at run time for what is, as you are at pains to point out, an otherwise trivial task - ie add some value.

It would be my belief that Volatility is a prime candidate ... other events could also account for the drag however given the context I would say that the likelihood of events being in place is small.

And for the record - optimisation is never irrelevant.  Advising people who are learning the basics of programming to ignore best practice and optimisation is non-sensical.
0
patrickabCommented:
DonkeyOte,

For such a small demonstration macro which executes without a problem in under a second there is nothing else worth considering. All other tweaks that are suggested are, in this instance, a waste of time and effort.

Optimisation of macros is one of those lovely-to-do things that only becomes relevant when there are genuine speed problems. In this case it really does not make sense to point to code optimisation nor ScreenUpdating as possibly helping.

However in larger more complex situations all the points you have raised are obviously relevant and worth consideration.

Patrick
0
NiklasMollerAuthor Commented:
Thanks everyone for many suggestions.
I just tried the script again. And yes, it takes 2,5 seconds (approx).
This is now on my computer nr 2 (also kind of new so computer power is not an issue).

However:
Regarding the volatile functions...
Im not sure what that means but I do have several sheets that have tons of these formulas in them:

=INDIRECT("'p1'!D28")+INDIRECT("'p2'!D28")+INDIRECT("'p3'!D28")+INDIRECT("'p4'!D28")+INDIRECT("'p5'!D28")+INDIRECT("'p6'!D28")

They sum up different values from different sheets.
Is that a volatile formula?

Could the slow performance be related to this?

Thanks

0
jppintoCommented:
YES!
0
NiklasMollerAuthor Commented:
@DonkeyOte:

Thanks for pointing me to that link. I tried inserting the optimization code before and after my code and it did help alot. Now it takes maybe half a second instead.

So the code now looks like this. (see code). And it is faster.
However, it still takes about 0,5 seconds, which still seems like alot...

Do the optimization measures turn off the calculation in the volatile formulas? or no?
Are they still there making it slow?

thanks
Sub Test()
    
    'Get current state of various Excel settings; put this at the beginning of your code

    screenUpdateState = Application.ScreenUpdating

    statusBarState = Application.DisplayStatusBar

    calcState = Application.Calculation

    eventsState = Application.EnableEvents

    displayPageBreakState = ActiveSheet.DisplayPageBreaks 'note this is a sheet-level setting

    'turn off some Excel functionality so your code runs faster

    Application.ScreenUpdating = False

    Application.DisplayStatusBar = False

    Application.Calculation = xlCalculationManual

    Application.EnableEvents = False

    ActiveSheet.DisplayPageBreaks = False 'note this is a sheet-level setting

    '>>your code goes here<<

     'loop over each sheet
    'sum the values of the same cell
    
    Dim currSumRow As Integer
    Dim currSumCol As Integer
    
    'loop each column in turn from left to right >>>

    For currSumCol = 5 To 40
        
        'nested: loop each row from top to bottom
            For currSumRow = 5 To 40
                
                'nested: loop each sheet from first to last
                    For Each sht In Sheets
                           Worksheets("test").Cells(currSumRow, currSumCol).Value = "testing"
                    Next sht

            Next currSumRow 'end row loop

    Next currSumCol 'end column loop


    'after your code runs, restore state; put this at the end of your code

    Application.ScreenUpdating = screenUpdateState

    Application.DisplayStatusBar = statusBarState

    Application.Calculation = calcState

    Application.EnableEvents = eventsState

    ActiveSheet.DisplayPageBreaks = displayPageBreaksState 'note this is a sheet-level setting
    
    
   
    
End Sub

Open in new window

0
patrickabCommented:
NiklasMoller,

>Could the slow performance be related to this?

INDIRECT() is a volatile formula, so with tons of such formulae the workbook will recalculate slowly. However that does not mean the macro in itself runs slowly. There is little or nothing that you can do to make the macro code run faster. What you can do is to use non-volatile functions or to consign the work to a macro instead - so placing values instead of volatile formulae in the cells.

Patrick
0
patrickabCommented:
Looks like I stand corrected...

Patrick
0
patrickabCommented:
This loop is redundant:

For Each sht In Sheets
   Worksheets("test").Cells(currSumRow, currSumCol).Value = "testing"
Next sht

Only this is needed:

Worksheets("test").Cells(currSumRow, currSumCol).Value = "testing"

Patrick
0
NiklasMollerAuthor Commented:
@patrickab:
Thanks for your help and clarifications.

> This loop is redundant:

Yes I am completely aware of that. Its a completely useless loop :)
It doesnt make any sense in a result-oriented way. But as said, thats not the point.
What amazed me was that it was very slow.

But thanks for pointing it out anyways. You were right in asuming I might not be aware of the nonsense of the loop :)

thanks!
0
NiklasMollerAuthor Commented:
No... something here is the matter.

Okay, so update to this whole problem:

I created a completely BLANK workbook. No volatile stuff. Nothing. Zip.
Renamed one of the sheet- tabs to "test".
inserted new module.
pasted code below

ran the code, it takes approx 5 seconds to complete!

Please note I have now removed the optimization code.
but still... is that not a very long time for filling a couple of hundred of cells with the string "testing" ???
(even considering that the loop is redundant and does too much?)

I mean. In AS3 Ive read tests where people can do a loop over 500,000 elements in an array and do .push() operations on them and still have the whole loop done in 0,5 seconds...?

hope you dont feel Im repeating myself.
please tell me if this behaviour is "normal"

thanks


Sub Test()

     'loop over each sheet
    'sum the values of the same cell
    
    Dim currSumRow As Integer
    Dim currSumCol As Integer
    
    'loop each column in turn from left to right >>>

    For currSumCol = 5 To 40
        
        'nested: loop each row from top to bottom
            For currSumRow = 5 To 40
                
                'nested: loop each sheet from first to last
                    For Each sht In Sheets
                           Worksheets("test").Cells(currSumRow, currSumCol).Value = "testing"
                    Next sht

            Next currSumRow 'end row loop

    Next currSumCol 'end column loop
    
End Sub

Open in new window

0
patrickabCommented:
NiklasMoller,

May I ask why you are using INDIRECT() for such a simple task as adding up values from different worksheets.

I believe, uless I have misunderstood the formula this could be replaced:

=INDIRECT("'p1'!D28")+INDIRECT("'p2'!D28")+INDIRECT("'p3'!D28")+INDIRECT("'p4'!D28")+INDIRECT("'p5'!D28")+INDIRECT("'p6'!D28")

with

=SUM('P1:P6'!D28)

Patrick
0
DonkeyOteCommented:
@Patrick, just so we're on the same page - I don't doubt your knowledge but equally you should not make assumptions regards the workings of a model to which you have no access nor the knowledge of others.

We can only base our responses on the facts to which we are privvy, in this instance Volatility would be the most obvious culprit with events next.

@Niklas

>Do the optimization measures turn off the calculation in the volatile formulas? or no?
Are they still there making it slow?

What the code does is

a) cache Application settings prior to executing the main code
b) modifies settings to their "optimium" values - eg manual calc, no screenupdate, disabled events etc...
c) restores the settings to their prior state (as cached)

So in answer to your question, the Volatility is removed at run-time (only), ie when you alter the cells contents via your iterative loop you're not invoking calculations of your volatile functions.

If you want to learn more about optimisation with formulae you can't do better than Charles Williams' site:

http://www.decisionmodels.com/calcsecretsi.htm

What I would say, based on the below:

>=INDIRECT("'p1'!D28")+INDIRECT("'p2'!D28")+INDIRECT("'p3'!D28")+INDIRECT("'p4'!D28")+INDIRECT("'p5'!D28")+INDIRECT("'p6'!D28")

is that there is really no need for INDIRECT in the above given you have no variables creating the range references - ie

=SUM('P1'!D28,'P2'!D28,'P3'!D28,'P4'!D28,'P5'!D28,'P6'!D28)

would work equally well without the volatility.... further - if sheets P1 to P6 are listed in sequence in the file you can use 3D SUM, eg:

=SUM('P1:P6'!D28)

As a general rule of thumb INDIRECT is only warranted if:

a) the sheet target is variable (or file path if used with open targets)

b) the name of the sheet is constant but the sheet is physically removed and replaced

If either of the above do not hold true then invariably where a variable reference is required a non-volatile INDEX function will suffice.

0
NiklasMollerAuthor Commented:
@patrickab:
=SUM('P1:P6'!D28)
thats a great tip! and probably one that I will have great use for in the future. thanks for pointing it out.

However, the issue at hand here is the slowness of the execution
0
DonkeyOteCommented:
@Patrick - I appear to have duplicated your comment re: 3D referencing / INDIRECT etc so apologies for that... regards optimisation I suggest we simply agree to disagree ;)
0
NiklasMollerAuthor Commented:
everyone... please note my updated comment starting with:
"No... something here is the matter."

thanks!
0
patrickabCommented:
NiklasMoller,

With a much larger loop, 35 x 35 = 1225,  as opposed to 5 x 5 = 25 as originally specified it is worth using:

Application.ScreenUpdating = False

Patrick

0
NiklasMollerAuthor Commented:
@patrickab:

using

Application.ScreenUpdating = False

really made a difference. Instead of 5 seconds it takes less than 0,5 seconds!

Thats enough efficiency for me, although i still think it feels kind of slow. :)

Thanks!
I will award some points now.
0
patrickabCommented:
>@Patrick - I appear to have duplicated your comment re: 3D referencing / INDIRECT etc so apologies for that... regards optimisation I suggest we simply agree to disagree ;)

No worries. I do it frequently due to a simple crossover between each of us pressing 'Submit' at much the same time.

I think we are much closer than you might imagine on the matter of code optimisation.  I believe there is a strong case for it but only where it matters. Dave Brett (brettdj) reported on a significant increase in speed simply by splitting an 'If condition and condition Then' clause into two separate If clauses. That filtered out those items that were not relevant in under half the time. There are of course many onther ways of optimising code, which you have covered. My only point was not to look in that direction for such a small macro.

Patrick
0
DonkeyOteCommented:
I wouldn't say that the performance is surpising - assume a file of 6 sheets, presently you're conducting:

36 * 6 * 36 -> 7776 iterations

and with each iteration you're writing back from VBE to native XL (repaint)

Leaving App settings as standard but reworking the loop to:

Sub Testing()
For Each sht In Sheets
    With Sheets("test")
        .Range("E5:AN40").Value = "testing"
    End With
Next sht
End Sub

You will find there is huge improvement - this is purely to demo the slowness of iteration.

When updating ranges en masse Arrays are generally quickest and processing ranges in their entirety via Evaluate is invariably quicker than Iteration.


0
DonkeyOteCommented:
edit: slowness of iteration & repaint etc...
0
patrickabCommented:
Well actually it's not 36 * 6 * 36 -> 7776 iterations

It's only 36 * 36 as there's only one worksheet named 'test'

Patrick
0
DonkeyOteCommented:
Patrick, per the code there is no restriction based on sheet name - all sheets are iterated however only one sheet (test) is ever written to - so 7776 iterations I'm afraid.
0
NiklasMollerAuthor Commented:
why dont we have a drink while were discussing this ;)
0
patrickabCommented:
...so 7776 iterations I'm afraid.

You're right. Remove the 'For Each sht In Sheets' loop and it executes in a fraction of the time - particularly if screenupdating is switched off.

Patrick
0
DonkeyOteCommented:
@Patrick - whilst I wholeheartedly agree with your (repeated) assertion that the sheet iteration serves no functional purpose the point remains per OP that:

>Yes I am completely aware of that. Its a completely useless loop :)
It doesnt make any sense in a result-oriented way. But as said, thats not the point.
What amazed me was that it was very slow.

I think the point regards avoiding / limiting iteration, disabling screen update etc have been made by both of us previously and truth be told I'm not sure what value repeating the comments has at this stage ?  
I would say it's fairly evident to all (at this stage) that reducing iterations (altogether where possible) and optimising code will lead to improved performance.

If nothing else I believe this thread bears testimony to the fact that new coders should always understand the basics of optimal coding and said techniques are never "irrelevant" - even if they may not make a huge % difference to performance to the current issue (as they did here) they will at some point.  Moreover understanding the basics will help them recognise why certain practices are to be avoided thereby improving their overall understanding of VBA - the how's and why's etc...
0
patrickabCommented:
DonkeyOte,

Oh dear, I have upset you as new member of EE. All the same welcome to EE.

I trust you now feel you have fully covered the matter.

Patrick
0
DonkeyOteCommented:
>Oh dear, I have upset you as new member of EE.

Patrick, not at all, you really think you can have that effect ? ;)

>I trust you now feel you have fully covered the matter.

On an open forum I trust I can critique your advice as you can (and indeed did) mine - albeit incorrectly.

Regardless of all of the above - thank you for the welcome - I am as you say new to this particular forum and thus far I have enjoyed every minute of the time spent here.... thanks to rorya and co. for suggesting I (and others) check it out !
0
patrickabCommented:
NiklasMoller,

BTW if you want to avoid loops altogether and create a fast macro then you could use code like this:

Sub tester()
Dim rng As Range

With Sheets("test")
    Set rng = Range(.Cells(5, 5), .Cells(40, 40))
End With
rng = "testing"

End Sub

or perhaps even:

Sub tester()
With Sheets("test")
    Range(.Cells(5, 5), .Cells(40, 40)) = "testing"
End With
End Sub

In these cases 'ScreenUpdating' is not needed to make either macro execute quickly.

Patrick
0
DonkeyOteCommented:
In the context of this example:

Sheets("test").Range("E5:AO40").Value = "testing"

is surely simplest of all.

It would be "good practice" (if not important) to release the range Object you've created in your first example.
0
patrickabCommented:
>It would be "good practice" (if not important) to release the range Object you've created in your first example.

In VBA that is not necessary as it takes care of native objects itself.

Patrick
0
patrickabCommented:
BTW the use of 'Value' is implicit in VBA so this:

Sub tester()
Sheets("test").Range("E5:AO40").Value = "testing"
End Sub

can be reduced to this:

Sub tester()
Sheets("test").Range("E5:AO40") = "testing"
End Sub

or even this:

Sub tester()
Sheets("test").[E5:AO40] = "testing"
End Sub

Patrick
0
DonkeyOteCommented:
Patrick, again we'll have to agree to disagree.

As I said (clearly) it will not improve performance but it's still a good "idea"

The topic has been covered previously by people far more knowledgeable than I

http://www.experts-exchange.com/Software/Office_Productivity/Office_Suites/MS_Office/Excel/Q_21489443.html

and the consensus would certainly be that from a memory perspective releasing the Objects is an irrelevance (these days), however, it is not always an irrelevance depending upon subsequent usage of the Object, to quote Zorvek from the above:

>The only cases where setting objects to nothing makes sense is of you are going to reuse a variable before it goes of scope or you are done with a variable and it is defined at the module or global level.

Releasing Objects does no harm and only reduces risk - on that basis I think it could be argued to be "good practice", no ?
0
patrickabCommented:
>Releasing Objects does no harm and only reduces risk - on that basis I think it could be argued to be "good practice", no ?

When zorvek and fanpages (who writes code extremely carefully and comprehensively) essentially agree that it is no longer necessary to set objects to nothing then I am more than willing to accept their word on the subject. On that basis it appears to be a superfluous practice. I personally never do so and I have never had problems of memory 'leakage'.

Patrick
0
DonkeyOteCommented:
If you re-read my prior post you will note that I stipulated the reason for releasing objects and it had nothing to do with memory leakage.

Hopefully we can agree on one thing - namely that this thread has deviated from the original question and has now become to all intents and purposes a little pointless ?

Suffice to say I do not agree with the greater majority of your posts on this thread nor you mine - and that's no bad thing IMO.
0
It's more than this solution.Get answers and train to solve all your tech problems - anytime, anywhere.Try it for free Edge Out The Competitionfor your dream job with proven skills and certifications.Get started today Stand Outas the employee with proven skills.Start learning today for free Move Your Career Forwardwith certification training in the latest technologies.Start your trial today
Microsoft Excel

From novice to tech pro — start learning today.

Question has a verified solution.

Are you are experiencing a similar issue? Get a personalized answer when you ask a related question.

Have a better answer? Share it in a comment.