Skip to content

Use global WScript object instead of creating it every time it's used #1976

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stenci opened this issue Aug 3, 2022 · 2 comments · Fixed by #1980
Closed

Use global WScript object instead of creating it every time it's used #1976

stenci opened this issue Aug 3, 2022 · 2 comments · Fixed by #1980
Milestone

Comments

@stenci
Copy link

stenci commented Aug 3, 2022

I wanted to create a pull request, but:

  • The signature is lost when I save the add-in
  • I don't know how to test it
  • I wasn't able to use the add-in from the forked repository (I think because something goes wrong when loading xlwings32-dev.dll)

So I did the change on the add-in installed by pip and I'm describing the changes and the reason why I did it here.


Executing CreateObject("Wscript.Shell") once and keeping a global object makes the execution of UDFs more than 3 times faster. On my computer with default configuration it goes from 0.013 down to 0.0038 seconds per call.


I have defined this UDF:

@xw.func
def my_function(x):
    return x + 1

I have added NCalls = NCalls + 1 before each CreateObject("Wscript.Shell") and ran this test:

Sub Test1()
  Dim I As Long, T0 As Single
  NCalls = 0
  T0 = Timer
  For I = 1 To 100
    my_function I
  Next I
  Debug.Print Timer - T0, NCalls
End Sub

The test shows that CreateObject("Wscript.Shell") is executed 16 times per UDF execution (with the default configuration I am using) and that the time required for calling 100 UDFs is 1.32 seconds:

 1.332031      1600 
 1.320313      1600 

After removing the calls to CreateObject("Wscript.Shell") and using a static object and running this test:

Sub Test2()
  Dim I As Long, T0 As Single
  T0 = Timer
  For I = 1 To 100
    my_function I
  Next I
  Debug.Print Timer - T0
End Sub

The test shows that the time required for calling 100 UDFs is 0.38 seconds:

 0.3867188 
 0.3828125 

Here is the diff (after manually removing parts that were different between the installed add-in and the one in the repository). I copied the WScript() function from one of my old add-ins, I've been using it for years:

diff --git a/xlwings/addin/Main.bas b/xlwings/addin/Main.bas
index 6a909aa..f3b391f 100644
--- a/xlwings/addin/Main.bas
+++ b/xlwings/addin/Main.bas
@@ -207,10 +207,8 @@ Function ExecuteWindows(IsFrozen As Boolean, PythonCommand As String, PYTHON_WIN
         ShowConsole = 0
     End If
 
-    Dim Wsh As Object
     Dim WaitOnReturn As Boolean: WaitOnReturn = True
     Dim WindowStyle As Integer: WindowStyle = ShowConsole
-    Set Wsh = CreateObject("WScript.Shell")
     Dim DriveCommand As String, RunCommand, condaExcecutable As String
     Dim PythonInterpreter As String, PythonDir As String, CondaCmd As String, CondaPath As String, CondaEnv As String
     Dim ExitCode As Long
@@ -274,7 +272,7 @@ Function ExecuteWindows(IsFrozen As Boolean, PythonCommand As String, PYTHON_WIN
         RunCommand = Chr(34) & PythonCommand & Chr(34) & " " & FrozenArgs & " "
     End If
     
-    ExitCode = Wsh.Run("cmd.exe /C " & DriveCommand & _
+    ExitCode = WScript.Run("cmd.exe /C " & DriveCommand & _
                        RunCommand & _
                        " --wb=" & """" & ActiveWorkbook.Name & """ --from_xl=1" & " --app=" & Chr(34) & _
                        Application.Path & "\" & Application.Name & Chr(34) & " --hwnd=" & Chr(34) & Application.Hwnd & Chr(34) & _
@@ -291,9 +289,6 @@ Function ExecuteWindows(IsFrozen As Boolean, PythonCommand As String, PYTHON_WIN
     On Error Resume Next
         Kill LOG_FILE
     On Error GoTo 0
-
-    ' Clean up
-    Set Wsh = Nothing
 End Function
 
 Public Function RunFrozenPython(Executable As String, Optional Args As String)
diff --git a/xlwings/addin/Utils.bas b/xlwings/addin/Utils.bas
index 2db5e05..5022aef 100644
--- a/xlwings/addin/Utils.bas
+++ b/xlwings/addin/Utils.bas
@@ -2,6 +2,12 @@ Attribute VB_Name = "Utils"
 Option Explicit
 #Const App = "Microsoft Excel" 'Adjust when using outside of Excel
 
+Function WScript(Optional CreateNew As Boolean) As Object
+  Static Value As Object
+  If CreateNew Or Value Is Nothing Then Set Value = CreateObject("WScript.Shell")
+  Set WScript = Value
+End Function
+
 Function IsFullName(sFile As String) As Boolean
   ' if sFile includes path, it contains path separator "\" or "/"
   IsFullName = InStr(sFile, "\") + InStr(sFile, "/") > 0
@@ -142,7 +148,6 @@ Sub ShowError(FileName As String, Optional Message As String = "")
     ' Shows a MsgBox with the content of a text file
 
     Dim Content As String
-    Dim objShell
     Dim ErrorSheet As Worksheet
 
     Const OK_BUTTON_ERROR = 16
@@ -170,8 +175,7 @@ Sub ShowError(FileName As String, Optional Message As String = "")
             Content = Content & vbCrLf
             Content = Content & "Press Ctrl+C to copy this message to the clipboard."
     
-            Set objShell = CreateObject("Wscript.Shell")
-            objShell.Popup Content, AUTO_DISMISS, "Error", OK_BUTTON_ERROR
+            WScript.Popup Content, AUTO_DISMISS, "Error", OK_BUTTON_ERROR
         #End If
     End If
 End Sub
@@ -197,10 +201,7 @@ Function ExpandEnvironmentStrings(ByVal s As String)
             ExpandEnvironmentStrings = s
         End If
     #Else
-        Dim objShell As Object
-        Set objShell = CreateObject("WScript.Shell")
-        ExpandEnvironmentStrings = objShell.ExpandEnvironmentStrings(s)
-        Set objShell = Nothing
+        ExpandEnvironmentStrings = WScript.ExpandEnvironmentStrings(s)
     #End If
 End Function
@fzumstein fzumstein moved this to TODO in xlwings planner Aug 3, 2022
@stenci stenci changed the title Use global Wscript object instead of creating it every time it's used Use global WScript object instead of creating it every time it's used Aug 3, 2022
@fzumstein fzumstein added this to the 0.27.12 milestone Aug 4, 2022
fzumstein added a commit that referenced this issue Aug 4, 2022
…#1980)

* Use global WScript object instead of creating it every time it's used, closes #1976

* added instructions on how to use xlwings vba edit [skip ci]
Repository owner moved this from TODO to Done in xlwings planner Aug 4, 2022
@fzumstein
Copy link
Member

Awesome, thanks! A few coments:

The signature is lost when I save the add-in

It is automatically signed when released

I don't know how to test it

The UDF tests are under tests/udfs but admittedly poorly documented

I wasn't able to use the add-in from the forked repository (I think because something goes wrong when loading xlwings32-dev.dll)

The version here needs to match the version of the dll. I've also added a few comments on how to edit the add-in: https://github.com/xlwings/xlwings/blob/main/DEVELOPER_GUIDE.md#addin

=> I think the real issue here is that it runs GetConfig multiple times every time a UDF is executed instead of just a single time when the interpreter is started. So probably lots of room for improvement...
See: https://github.com/xlwings/xlwings/blob/main/xlwings/addin/Main.bas#L447-L463

@stenci
Copy link
Author

stenci commented Aug 5, 2022

I think the real issue here is that it runs GetConfig multiple times every time a UDF is executed instead of just a single time when the interpreter is started

I agree.

In my add-ins I have something similar, where the configuration is spread across one Config sheet, environment variables, http rest apis, etc.

I usually have one Configuration class that is instantiated once in a module with Public Config As New Configuration and used as a singleton.

When the object is instantiated, it immediately reads all the values from the Config sheet into a dictionary. Reading them lazily would be a waste of time, because it would need to search each one the first time it's required, while loading them all requires only one scan.

Some settings are loaded lazily, for example the ones coming from http rest apis, because it would be too slow to read them all, and one never needs all of them, and sometimes the full list is not even known.

The class has methods like:

  • Config.Setting(SettingName) -> returns the value of SettingName
  • Config(SettingName) -> same as above, Setting is the default property thanks to "Attribute Setting.VB_UserMemId = 0"
  • Config.ToJson -> show the full list of settings, very useful for settings that are expanded, evaluated, etc. I use it often with WriteTextFile Config.ToJson, "C:\config.json"
  • Config.Reload -> resets the dictionary and runs the initialization, This is called on all the entry points of the add-in, so the configuration is always updated. For example when clicking on any button, when the Config sheet is changed or when events that use the configuration are triggered. I also have a ribbon button to force the reload, useful when changing a text file or something similar, before using UDFs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants