19
\$\begingroup\$

I have an Outlook PST file of a pretty big size (13 GB). For some indexing purpose I needed to scan through the full PST file to get all the PR_SEARCH_KEY and then write into a text file. This recursive procedure is used to loop through all folders and subfolders to get the PR_SEARCH_KEY from each email and non-email item. It took nearly 30 minutes to complete the full scan and writing into the text file.

How can I make this faster, such as for completing the process within 5 minutes? Is there any efficiency I'm missing here? I'm very new to Outlook and VB.NET.

Dim dict = New Dictionary(Of String, String)

Sub PrepareIndexing()
    Dim oApp As Outlook.Application
    Dim objName As Outlook.NameSpace
    Dim sFolder As Outlook.Folder        
    oApp = CreateObject("Outlook.Application")
    objName = oApp.GetNamespace("MAPI")
    sFolder = objName.Folders.Item("pstname")

    dict.Clear()

    Call InitIndexing(sFolder)

    For Each d As KeyValuePair(Of String, String) In dict
        Using outputFile As New StreamWriter("E:\" & Convert.ToString("file.txt"), True)
            outputFile.WriteLine(d.Key & "," & d.Value)
        End Using
    Next
    dict.Clear()        
End Sub


Sub InitIndexing(f As Outlook.Folder)
    Dim PropName, skey As String
    Dim oPA As Outlook.PropertyAccessor
    PropName = "http://schemas.microsoft.com/mapi/proptag/0x300B0102" 'Pr_Search_Key

    If f.Folders.Count > 0 Then
        For c = 1 To f.Folders.Count
            Dim Folder As Outlook.Folder = f.Folders.Item(c)
            Dim r As String = Folder.FolderPath
            For Each ml In Folder.Items
                oPA = ml.PropertyAccessor
                skey = oPA.BinaryToString(oPA.GetProperty(PropName))
                If Not dict.ContainsKey(skey) Then
                    dict.Add(skey, r)
                End If
            Next
            Call InitIndexing(Folder)
        Next
    End If
End Sub
\$\endgroup\$
  • \$\begingroup\$ I changed the language tag from vba to vb.net. VBA does not have a Using statement, nor does it have strongly typed CreateObject calls ... \$\endgroup\$ – Vogel612 Sep 27 '17 at 11:05
  • \$\begingroup\$ Thanks, I'm a real novice with little knowledge in vba or vb.net \$\endgroup\$ – Coder_v0.01 Sep 27 '17 at 11:37
  • \$\begingroup\$ I would first start to separate your logic into pieces. Make a method that gets all the folders, an other that parse the folder... That way, you will know which method is taking to most time. Is it the folder search, the BinaryToString, the saving to the file at the end? \$\endgroup\$ – the_lotus Sep 27 '17 at 12:02
  • \$\begingroup\$ @Vogel612 VBA can absolutely cast the output of CreateObject to whatever type it needs to work with. The reason why you only declare the object variable As Object is because when you use CreateObject to create an instance of an object via its registered ProgId, you typically don't have a reference to the type library it's defined in. Be it VBA or VB.NET, there's no reason to CreateObject("Outlook.Application") when you can already do New Outlook.Application. \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 13:53
  • 5
    \$\begingroup\$ What's Call doing in VB.NET code? The explicit Call syntax was already obsolete 17 years ago in VBA! \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 13:57
21
\$\begingroup\$

I would assume that this

For Each d As KeyValuePair(Of String, String) In dict
    Using outputFile As New StreamWriter("E:\" & Convert.ToString("file.txt"), True)
        outputFile.WriteLine(d.Key & "," & d.Value)
    End Using
Next  

is the bottleneck of your code. You are opening, writing to and closeing the file for each KeyValuePair of the dictionary.

Change it like

Using outputFile As New StreamWriter("E:\" & Convert.ToString("file.txt"), True)
    For Each d As KeyValuePair(Of String, String) In dict
            outputFile.WriteLine(d.Key & "," & d.Value)
    Next
End Using  

and it should be running much faster.

Btw, why do you use Convert.ToString("file.txt") ?


InitIndexing()

by changing the condition of the f.Folders.Count to check if Count = 0 you could return early from the Sub and therefor save one level of indentation like so

If f.Folders.Count = 0 Then Exit Sub

For c = 1 To f.Folders.Count
    Dim Folder As Outlook.Folder = f.Folders.Item(c)
    Dim r As String = Folder.FolderPath
    For Each ml In Folder.Items
        oPA = ml.PropertyAccessor
        skey = oPA.BinaryToString(oPA.GetProperty(PropName))
        If Not dict.ContainsKey(skey) Then
            dict.Add(skey, r)
        End If
    Next
    Call InitIndexing(Folder)
Next  

but you have bigger problems here: Naming ! You shouldn't use abbreviations to name things. If you come back to this code in 3 months you won't know the meaning of oPA, r or objName. You should always use meaningful names to name things.

\$\endgroup\$
  • \$\begingroup\$ I've changed my code as you've suggested (all of the points) but still it took nearly 30 minutes. I'm thinking about these point if those are consuming most of time to process : 1) Using Dictionary to store data instead of Array or anything else, 2) Getting skey (PR_SEARCH_KEY) by accessing Outlook Property for which this is maybe only way. I don't know if there are any better ways for these two point . Please suggest. Thanks. \$\endgroup\$ – Coder_v0.01 Sep 27 '17 at 11:35
  • 2
    \$\begingroup\$ Compile in Release mode and execute the compiled app. Should be much faster. \$\endgroup\$ – Heslacher Sep 27 '17 at 12:44
  • 3
    \$\begingroup\$ After having tried it, I'm inclined to believe that outlook interop is just plain slow (same as with office and excel). \$\endgroup\$ – Johnbot Sep 27 '17 at 13:07
  • 4
    \$\begingroup\$ @Hayat the problem isn't VB.NET, it's the COM API. \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 23:18
  • 1
    \$\begingroup\$ I am not so shocked than a 13g file take 30mn to process, I mean if we consider already that just reading with a classic hard drive goes at 30M/s, that makes already 5mn just to read the file (without anything else hitting on the hard drive, anti-virus are you here ? :p). Furthermore you're also writing. \$\endgroup\$ – Walfrat Sep 28 '17 at 13:25
16
\$\begingroup\$

COM interop is in itself a performance hit, but even when accessing it through COM (e.g. in VBA), the Outlook type library / object model is notoriously, painfully slow. If performance is a requirement I would consider parsing the PST file without involving the Outlook object model, e.g. with something like PST-Parser (no affiliation, just an OSS project I found googling for PST parsers).

So, given a huge portion of the performance problem is primarily due to the approach (Outlook interop), I'm not going to review your code from a performance standpoint - but maintainability-wise, there are a number of issues.

  • Implicit access modifiers: Members in VB.NET are Public by default, as they were in its VB6 ancestor. This is confusing, because in a lot of other programming languages, module members are Private by default. In any case, avoid implicit access modifiers; specify them explicitly, leaving no room for ambiguity: if it's meant to be invoked from outside the module, make it Public. If it's meant to be invoked from within the module, make it Private.

  • Implicit ByVal modifiers: Parameters in VB.NET are passed by value unless specified otherwise. This is in stark contrast with VB6/VBA code, where parameters are passed by reference by default. A reader that's constantly context-switching between VBA and VB.NET code will have a much easier time if these modifiers are explicit everywhere - if you don't know what ByVal vs ByRef means, I encourage you to lookup the difference: it's a rather important thing to know about.

  • Obsolete syntax: The explicit Call syntax was already obsolete in VB6/VBA, over 17 years ago. There is no reason whatsoever to carry that keyword into your VB.NET code.

  • Inconsistent variable scopes: in VBA the smallest possible scope for a variable was member-level. In VB.NET blocks are also scopes, which means if you're declaring a local variable at procedure/member-level, and only using it inside a For loop iteration, then the scope of that variable is wider than it needs to be, and that makes the code harder to follow. For example oPA and skey in InitIndexing really belong to a single iteration in that For loop: them outliving the loop scope makes no sense. Declare variable as close as possible to their usage; don't have a "wall of declarations" at the top of a procedure.

A word about this:

Dim PropName, skey As String

In VBA that would declare PropName as an implicit Variant, and skey as a String. VB.NET "fixed" that, so both PropName and skey are strings - but while it's perfectly legal to declare more than one variable with a single instruction, it's confusing and useless to do so, especially since PropName appears to be constant and skey in a wider scope than it needs to be - I'd much rather see this:

Const PropertyName As String = "http://schemas.microsoft.com/mapi/proptag/0x300B0102"

Avoid single-letter variable names - use meaningful, pronounceable names.

  • f is a folder - why not call it folder?
  • skey looks like it's "key" with an "s-for-string" Systems Hungarian prefix that has absolutely zero value. What's that key representing? Looks like it's the string representation of a binary property. So, it's a property. How about propertyKey then?
  • oPA is really propertyAccessor, or perhaps accessor is good enough in this context. That o prefix reeks of System Hungarian ("o-for-object"? drop it! Everything is an object anyway!).
  • c means absolutely nothing. it's a folderIndex, but you're iterating an object collection, and doing that with a For Each loop would be much faster than with a For loop, which is faster for iterating arrays. Besides,1 isn't this much sexier?

    For Each subFolder In folder.Folder
    
  • r is again some seemingly randomly-picked single-letter name without a meaning. It stands for Folder.FolderPath... so path seems like a much better name. So that would be Dim path As String = subFolder.FolderPath, but that seems like a superfluous allocation - r is only used once, and conditionally at that; so you're unconditionally accessing a costly COM interop object to assign a value that you're only going to use if the key doesn't already exist in your dictionary - I'd move that variable to the conditional scope it belongs to.

  • ml leaves me wondering what it did to deserve a 2-letter name. You're iterating folder items, so item or child seems a much better name to use.

The scope of dict feels wrong too: InitIndexing is really GetAllFolderItems, and it should be a Function that returns a dictionary rather than populating one that's outside its scope.

But then, when it's used, it's used as an IEnumerable(Of KeyvaluePair) as the items are being iterated - you gain absolutely nothing by using a dictionary here, and the Key and Value of each item could very well be concatenated right there on-the-spot.

GetAllFolderItems / InitIndexing should be a Function that returns an IEnumerable(Of String), and with the awesomeness of Yield Return you can even "yield" every item to the client loop and issue the results as they are being iterated:

Yield Return accessor.BinaryToString(accessor.GetProperty(PropertyName)) & ", " & subFolder.FolderPath

I'm not clear on exactly how it would be possible for the conditional to ever evaluate to False (i.e. how could a key ever be duplicated?).

With Yield Return you'd need to handle the recursive results a bit differently:

Dim subItem As String
For Each subItem In GetAllFolderItems(subFolder)
    Yield Return subItem
Next

And with that, the calling code can start enumerating the results well before every single item is known. Next step would be to collect these results asynchronously, but I'm not sure there'd be any real benefit to it (other than not freezing your app while it's collecting the data), since COM interop needs to marshal everything to one single thread anyway, and typically doesn't quite play well with concurrent or cross-thread accesses.

In any case, without Yield Return you're iterating everything twice over; with it, you iterate the results once, as they're being returned - it should make a little bit of a difference.


1 That was true in VB6/VBA, but no longer with .NET.

\$\endgroup\$
  • 3
    \$\begingroup\$ +1 "COM Interop is a performance hit itself".... ooooo ya. \$\endgroup\$ – GibralterTop Sep 27 '17 at 17:52
  • 1
    \$\begingroup\$ Hmm, I know the bit about For vs ForEach is correct for VBA, but not sure if it applies to VB.NET. \$\endgroup\$ – Mathieu Guindon Sep 27 '17 at 20:11
  • \$\begingroup\$ @Mat'sMug If you mean the bit about speed of one vs the other, I think it's quite irrelevant. The major selling point of ForEach is that it removes syntax clutter, so using it is almost always better if you don't need the extra enumerator. \$\endgroup\$ – BgrWorker Sep 28 '17 at 9:48
  • \$\begingroup\$ @BgrWorker That's what I thought. In VBA iterating an object collection with a for loop, is something like 27 times slower than using a foreach! \$\endgroup\$ – Mathieu Guindon Sep 28 '17 at 10:36

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.