Perf wrapper for Excel VBA

Question

Spoiler

This code is actually BAD for performances! So don't use it! ;)

---

For a while now, I've been using this wrapper to avoid retyping most of it :

Public Sub PerfWrap(SubNameToRun As String, _
                    Optional ByVal DispStatusBar As Boolean = False, _
                    Optional ArgumentsToPass As String = vbNullString)
    Dim aWB As Workbook, _
        ActiveSH As Worksheet, _
        ScreenUpdateState As Boolean, _
        StatusBarState As Boolean, _
        CalcState As XlCalculation, _
        EventsState As Boolean, _
        DisplayPageBreakState As Boolean
    Set aWB = ActiveWorkbook
    Set ActiveSH = aWB.ActiveSheet
    DoEvents
    With Application
        ScreenUpdateState = .ScreenUpdating
        StatusBarState = .DisplayStatusBar
        CalcState = .Calculation
        EventsState = .EnableEvents
        DoEvents
        .ScreenUpdating = False
        .DisplayStatusBar = DispStatusBar
        .Calculation = xlCalculationManual
        .EnableEvents = False
    End With
    DisplayPageBreakState = ActiveSH.DisplayPageBreaks
    ActiveSH.DisplayPageBreaks = False

    If ArgumentsToPass <> vbNullString Then
        Application.Run SubNameToRun, ArgumentsToPass
    Else
        Application.Run SubNameToRun
    End If

    DoEvents
    ActiveSH.Activate
    ActiveSH.DisplayPageBreaks = DisplayPageBreakState
    With Application
        .ScreenUpdating = ScreenUpdateState
        .DisplayStatusBar = StatusBarState
        .StatusBar = vbNullString
        .Calculation = CalcState
        .EnableEvents = EventsState
    End With
End Sub

I use it like this :

Public Sub Launcher_CopyAndUpdate()
    Call PerfWrap("CopyAndUpdate_Templates", True)
End Sub

Or to pass arguments to the runned procedure :

Public Sub Launcher_Update()
    Call PerfWrap("Update_Templates", True, "Arg1, Arg2")
End Sub

---

I was wondering if :

• it can be improved regarding performances
• converted as a function to return the value sent by the runned function

Anyhow, any input or opinion is very welcome! :)

Answer 1

First, I'd like to echo @Mat'sMug's distaste for your multi-lined Dim statements. There's absolutely no reason to do this at all having them prefixed with Dim makes it completely obvious that I'm looking at a declaration block. It took me a good minute to realize that it wasn't a continuation of Sub declaration, because the Dim literally disappears into a huge wall of code. You aren't saving any time typing it either - last time I checked, Dim and , _ were both 3 characters, and the former is a lot easier to type on a qwerty keyboard.

---

The other answers hint around the performance, but I'll just come right out and say it. Your Sub should really be named the AntiPerfWrap. First, every single call that Application.Run makes is late bound - even for Excel objects. It has to be, because like CallByName, it is forced to call IDispactch.GetIDsOfNames on itself. This is always slower than an early bound call - period, end of discussion.

---

You always cache the ActiveWorkbook and ActiveSheet regardless of whether it is necessary or not. I would guess that roughly 1% of all the code I've written requires this. Same thing with .Calculation. Same thing with .EnableEvents. Do I write another PerfWrapWithEvents if I need that turned on? Or maybe PerfWrapWithEventsAllowCalculation? None of this work is free, and it seems like a jumbled collection of "this will make your code go faster anecdotes" collected from SO comments. The cargo cult has found thier messiah in this wrapper.

---

DoEvents is not free. In fact it's the complete opposite. It yields the processor to every other thread that needs to clear it's event queue. It also makes absolutely no sense to call DoEvents when you've explicitly disabled 95% of the functionality that would make the call useful.

---

Brass tacks time - let's see how much overhead this wrapper really adds. This is the called routine:

Public Sub UnderTest()
    Dim i As Long
    For i = 1 To 10
        Debug.Print i
    Next
End Sub

And the baseline benchmark:

Public Sub BenchmarkOne()
    Dim starting As Single
    starting = Timer

    Dim i As Long
    For i = 1 To 100
        Sheet1.UnderTest
    Next

    Debug.Print "Unwrapped: " & Timer - starting
End Sub

Unwrapped: 0.8515625

Public Sub BenchmarkTwo()
    Dim starting As Single
    starting = Timer

    Dim i As Long
    For i = 1 To 100
        PerfWrap "Sheet1.UnderTest"
    Next

    Debug.Print "Wrapped: " & Timer - starting
End Sub

Wrapped: 6.492188

Ouch.

---

Conclusion - Burn it. Burn it with fire. The premise behind the code is that there is a common set of operations that you can do to "speed up" code. In fact, this is never true. In reality, you are turning off Excel functionality that can slow down code. There's a big difference. If you approach performance issues by looking outside of code that you've written, you're starting out looking in the wrong direction. A catch-all "solution" for performance simply doesn't exist, so I wouldn't pretend to be providing it. The best way to look at it is to imagine that any code under performance review to be copied and pasted in place of this...

If ArgumentsToPass <> vbNullString Then
    Application.Run SubNameToRun, ArgumentsToPass
Else
    Application.Run SubNameToRun
End If

...then do a code review on the entire thing. Ask yourself, "In the context of the full Sub, what is the reason for every single line of code other than part above"? I'm guessing that in roughly 99.99% of cases, there won't be a good answer for all of them.

Answer 2

You're inverting the caller/callee chain here. There are pro's and con's, but IMO more con's than pro's - Application.Run is a performance hit in the first place, and with the stringly-typed parameters you lose compile-time validation of your code... which I would do anything to try and preserve as much as possible.

Another downside is that it's not going to work on class methods, so you can't have an object that's responsible for interacting with a worksheet - may or may not be a problem, but utility code shouldn't be driving design decisions of its calling code that way.

---

This declaration is annoying:

Dim aWB As Workbook, _
    ActiveSH As Worksheet, _
    ScreenUpdateState As Boolean, _
    StatusBarState As Boolean, _
    CalcState As XlCalculation, _
    EventsState As Boolean, _
    DisplayPageBreakState As Boolean

You (or a maintainer) can't add a comment on any of those. This is probably going to break syntax highlighting in this post, but...

Dim aWB As Workbook, _ 'a comment here is illegal
    ActiveSH As Worksheet, _
    ScreenUpdateState As Boolean, _
    StatusBarState As Boolean, _
    CalcState As XlCalculation, _
    EventsState As Boolean, _
    DisplayPageBreakState As Boolean

Here's how the VBE sees it:

And if you manage to squeeze a legit comment in there, well you better hope you have Option Explicit specified:

Dim aWB As Workbook, _
    ActiveSH As Worksheet, _
    ScreenUpdateState As Boolean 'oh, and this one is legal, but... _
    StatusBarState As Boolean, _
    CalcState As XlCalculation, _
    EventsState As Boolean, _
    DisplayPageBreakState As Boolean

I would simply avoid cramming multiple declarations in a single statement, there's no real reason to do that... and there are real reasons to avoid it:

Variables should be declared close to their first use anyway, not in a wall of declarations at the top of the procedure.

FWIW with Rubberduck (a COM add-in for the VBE that I [and other fellow VBA reviewers] is [are] working on), that's a one-click fix:

That instantly turns the statement into this:

Dim aWB As Workbook
Dim ActiveSH As Worksheet
Dim ScreenUpdateState As Boolean
Dim StatusBarState As Boolean
Dim CalcState As XlCalculation
Dim EventsState As Boolean
Dim DisplayPageBreakState As Boolean

---

You don't need the the aWB local variable at all (a terrible name IMO), since you already have ActiveSH (which I'd probably rename to originalActiveSheet), and its .Parent member points to the originally active workbook.

---

It's not clear why SubNameToRun and ArgumentsToPass are passed ByRef (implicitly), when DispStatusBar is explicitly ByVal - all parameters could very well be ByVal here.

Actually - and given the 30 optional parameters of Application.Run that would look much uglier, but ArgumentsToPass would make a much friendlier API if it were a ParamArray parameter; that way you could keep type safety for your arguments, which means you could pass arguments that can't be implicitly or explicitly converted to a String - e.g. a Range object.

Not being able to pass an object parameter is a serious showstopper limitation IMO.

---

I think it's a nice idea, but it's way too limited. A more flexible solution would be a simple ToggleWaitMode procedure that toggles ScreenUpdating, DisplayStatusBar, Calculation, EnableEvents, allows specifying an optional StatusBar message, and heck, that toggles the Cursor to/from xlWait as well.

You could even wrap that toggle behavior in a class, encapsulate the state (although that's a leaky abstraction, these things are all global really), and use the Class_Terminate handler to revert back to original state, that way the calling code couldn't even "forget" to toggle it back:

With WaitWorker.Init(statusText:="Please wait...", disableEvents:=True)

    'do stuff

End With

That way you don't need to do Application.Run, so it works with class module callers, and you're not passing a stringly-typed list of parameters either, so the caller can still do everything it wants: you're not interfering with the caller.

Answer 3

I'm uncertain of the efficiencies in a single wrapper like this. In developing applications with multiple modules and classes, I'm often needing to disable screen updates and such but without extremely careful planning I used to always step on my own toes. So I developed two routines that allow me to disable and enable Excel application level updates and give me enough tracking debug.

Using the calls becomes as easy as sprinkling these calls around:

Sub SomeImportantWork()
    DisableUpdates
    '--- do stuff
    OtherImportantWork
    EnableUpdates
End Sub

Sub OtherImportantWork()
    DisableUpdates debugMsg:="enter OtherImportantWork"
    '--- work more stuff
    EnableUpdates debugMsg:="leave OtherImportantWork"
End Sub

My idea has been to not worry about whether I've call disable/enable, but only to make sure those calls are paired around any code that requires that protection.

At the module level, I define a structure and a private variable to hold any previous state information:

Option Explicit

Private Type SavedState
    screenUpdate As Boolean
    calculationType As XlCalculation
    eventsFlag As Boolean
    callCounter As Long
End Type
Private previousState As SavedState
Private Const DEBUG_MODE = True

Then when I want to disable application updates, I call:

Public Sub DisableUpdates(Optional debugMsg As String = "", _
                          Optional forceZero As Boolean = False)
    With Application
        '--- capture previous state if this is the first time
        If forceZero Or (previousState.callCounter = 0) Then
            previousState.screenUpdate = .ScreenUpdating
            previousState.calculationType = .Calculation
            previousState.eventsFlag = .EnableEvents
            previousState.callCounter = 0
        End If

        '--- now turn it all off and count
        .ScreenUpdating = False
        .Calculation = xlCalculationManual
        .EnableEvents = False
        previousState.callCounter = previousState.callCounter + 1

        '--- optional stuff
        If DEBUG_MODE Then
            Debug.Print "Updates disabled (" & previousState.callCounter & ")";
            If Len(debugMsg) > 0 Then
                Debug.Print debugMsg
            Else
                Debug.Print vbCrLf
            End If
        End If
    End With
End Sub

And the mirroring method to re-enable is:

Public Sub EnableUpdates(Optional debugMsg As String = "", _
                         Optional forceZero As Boolean = False)
    With Application
        '--- countdown!
        If previousState.callCounter >= 1 Then
            previousState.callCounter = previousState.callCounter - 1
        Else
            '--- shouldn't get here
            Debug.Print "EnableUpdates ERROR: reached callCounter = 0"
        End If

        '--- only re-enable updates if the counter gets to zero
        '    or we're forcing it
        If forceZero Or (previousState.callCounter = 0) Then
            .ScreenUpdating = previousState.screenUpdate
            .Calculation = previousState.calculationType
            .EnableEvents = previousState.eventsFlag
        End If

        '--- optional stuff
        If DEBUG_MODE Then
            Debug.Print "Updates enabled (" & previousState.callCounter & ")";
            If Len(debugMsg) > 0 Then
                Debug.Print debugMsg
            Else
                Debug.Print vbCrLf
            End If
        End If
    End With
End Sub

As you can see, I have a private module flag to turn on debug messages (DEBUG_MODE) if I need them and turn them off for production. You can turn this into a compiler directive flag if you feel you need a (small) runtime performance boost.

The callCounter lets me keep track of my "updates call stack". This is especially usedful when I started calling some library routines I had and those calls got rather deep. It certainly helped to track it down.'

Also, I can go into the Immediate window at any time and type EnableUpdates(forceZero=True) and get back to a known state manually.