2
\$\begingroup\$

I have a Component that is used for all Active Directory queries.

The function below is used heavily perhaps even milions of times each day.

Performance is a feature!

It's used on servers and I want to reduce memory press on these servers (when the GC is collecting the server websites etc are not responding... well of course ther are perhaps even bigger sinners in that case - but I write this in VB.NET and because of that the code has to be better than ordinary).

  1. I can trust the format so I don't have to check.

  2. dn = distinguishedName

  3. Data: "CN=joedoe0101,OU=User,OU=Cardiff Electric,OU=Company,DC=root,DC=usa,DC=net"

  4. Result: "Cardiff Electric"

Below I have two examples that not quite does what I want:

Friend Shared Function GetOrganization(dn As String) As String
    Return dn.Split(","c)(2).Substring(3)
End Function

Or this

Friend Shared Function GetOrganization(dn As String) As String
    Return dn.Substring(dn.IndexOf(","c) + 1).Split(","c)(1).Substring(3)
End Function

Is there a better solution?

Is there a solution to also get the "Company" ?

\$\endgroup\$
  • \$\begingroup\$ Do you have proof that this small piece of code is the culprit? \$\endgroup\$ – Greg Burghardt Dec 30 '14 at 22:17
  • \$\begingroup\$ No proof. And probalys is'nt the culprit. I have worse examples that could be helped by a "good" answer to above question. And still think of perhaps 250 millions split in a Year. \$\endgroup\$ – Yay Dec 30 '14 at 22:19
  • 2
    \$\begingroup\$ Performance optimizations are such a hit and miss thing. You are better off spending time researching how IIS and the .NET framework manages memory. Look into performance profiling software. Services like New Relic can really help. \$\endgroup\$ – Greg Burghardt Dec 31 '14 at 1:55
  • \$\begingroup\$ Of course You are right. Still I have to go to the bottom with this so I then can leave It. And in this case this is my responsibility while IIS is not. Given the splendid answer from Heslacher I can perhaps even go with my original solution with split. But to learn You must be extreme and then perhaps take two steps back. Before I didn't know how - now I do. If it's Worth it (given the lesser readability of the code) I then can decide. \$\endgroup\$ – Yay Dec 31 '14 at 3:28
  • 1
    \$\begingroup\$ What you can / can't do after receiving answers \$\endgroup\$ – Heslacher Dec 31 '14 at 11:08
2
\$\begingroup\$

You basically have 3 other possibilities

  1. Using Split() like you did, but different

    Private Const CommaOUEqual As String = ",OU="
    Friend Shared Function GetOrganization(dn As String) As String
        Return dn.Split({CommaOUEqual}, StringSplitOptions.None)(2)
    End Function  
    
  2. Using IndexOf() similiar like you did, nut without Split()

    Private Const CommaOUEqual As String = ",OU="
    Friend Shared Function GetOrganization(dn As String) As String
        Dim index As Integer = dn.IndexOf(CommaOUEqual) + 4
        index = dn.IndexOf(CommaOUEqual, index) + 4
        Return dn.Substring(index, dn.IndexOf(CommaOUEqual, index) - index)
    End Function  
    
  3. Using Regex

    Private Shared regex As Regex = New Regex(",OU=(.+?),OU=(.+?),OU=", RegexOptions.Compiled)
    Friend Shared Function GetOrganization(dn As String) As String
        Return regex.Match(dn).Groups(2).ToString()
    End Function  
    

Timing these 3 methods together with the 2 methods fromn you results after 500.000 iteration in

yours

  1. ~340 ms (Split())
  2. ~330 ms (IndexOf() + Split())

mine

  1. ~460 ms (Split())
  2. ~250 ms (IndexOf())
  3. ~900 ms (Regex)

So in the worst case for assuming 5.000.000 calls in 24 hours this will result for the worst case in 9 seconds and for the best case in 2.5 seconds.

But this are still only micro gain in performance.

But wait, we can do faster

Friend Shared Function GetOrganization(dn As String) As String
    Dim firstHit As Boolean = False
    Dim secondHit As Boolean = False
    Dim startIndex As Integer = 0
    For i As Integer = 6 To dn.Length - 3
        If dn.Chars(i) = "="c AndAlso dn.Chars(i - 1) = "U"c AndAlso dn.Chars(i - 2) = "O"c Then
            If Not secondHit AndAlso firstHit Then
                startIndex = i + 1
                secondHit = True
                Continue For
            End If
            If secondHit Then
                Return New String(dn.ToCharArray(startIndex, i - startIndex - 3))
            End If
            firstHit = True
        End If
    Next
    Return String.Empty
End Function  

This will run for 500.000 iterations in

~70 ms

Using

Return dn.Substring(startIndex, i - startIndex - 3)

instead of

Return New String(dn.ToCharArray(startIndex, i - startIndex - 3))

will improve the speed to about

~60 ms

if you need to parse more than one item of the given string we should use this to its own class and remove the Shared.

Friend Class ParsedLDAP
    Friend Enum LDAPComponent
        OU = 1
        DC = 2
        CN = 4
        ALL = 7
    End Enum
    Public Property CommonNames As IList(Of String)
    Public Property OrganisationalUnits As IList(Of String)
    Public Property DomainComponents As IList(Of String)
    Private Sub New()
        CommonNames = New List(Of String)
        OrganisationalUnits = New List(Of String)
        DomainComponents = New List(Of String)
    End Sub
    Public Sub New(dn As String)
        Me.new(dn, LDAPComponent.ALL)
    End Sub
    Public Sub New(dn As String, parseComponent As LDAPComponent)
        Me.New()
        ParseString(dn, parseComponent)
    End Sub

    Private Const reversedOU As String = "UO"
    Private Const reversedDC As String = "CD"
    Private Const reversedCN As String = "NC"
    Private Const equalSign As Char = "="c
    Private Sub ParseString(dn As String, parseComponent As LDAPComponent)
        Dim isNotOU As Boolean = ((parseComponent And LDAPComponent.OU) <> LDAPComponent.OU)
        Dim isNotDC As Boolean = ((parseComponent And LDAPComponent.DC) <> LDAPComponent.DC)
        Dim isNotCN As Boolean = ((parseComponent And LDAPComponent.CN) <> LDAPComponent.CN)

        If (isNotOU AndAlso isNotDC AndAlso isNotCN) Then Return

        Dim currentComponent As LDAPComponent
        For i As Integer = 2 To dn.Length - 1
            If dn.Chars(i) <> equalSign Then Continue For

            Dim startIndex As Integer = i + 1
            Select Case dn.Chars(i - 1)
                Case reversedOU.Chars(0)
                    If isNotOU Then Continue For
                    If dn.Chars(i - 2) <> reversedOU.Chars(1) Then Continue For
                    currentComponent = LDAPComponent.OU
                Case reversedDC.Chars(0)
                    If isNotDC Then Continue For
                    If dn.Chars(i - 2) <> reversedDC.Chars(1) Then Continue For
                    currentComponent = LDAPComponent.DC
                Case reversedCN.Chars(0)
                    If isNotCN Then Continue For
                    If dn.Chars(i - 2) <> reversedCN.Chars(1) Then Continue For
                    currentComponent = LDAPComponent.CN
                Case Else
                    Continue For
            End Select

            While (i < dn.Length - 1 AndAlso dn.Chars(i + 1) <> equalSign)
                i += 1
            End While

            If i = dn.Length - 1 Then i += 3 ' This is needed, because otherwise the last item can't be found

            Dim currentValue As String = dn.Substring(startIndex, i - startIndex - 2)
            Select Case currentComponent
                Case LDAPComponent.OU
                    OrganisationalUnits.Add(currentValue)
                Case LDAPComponent.DC
                    DomainComponents.Add(currentValue)
                Case LDAPComponent.CN
                    CommonNames.Add(currentValue)
            End Select

        Next
    End Sub
End Class  

Because I assume that you maybe have millions of calls where you need e.g the desired OU but for a much smaller number of distinct dn's I would suggest to store the results in a Dictionary(Of String, ParsedLDAP) to first lookup if the dn has been parsed by calling Dictionary(Of TKey,TValue).TryGetValue().

\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer especially that you have timings ! But still could there be an even better solution ? I give You my Point and wait some weeks Before accepting as an answer. \$\endgroup\$ – Yay Dec 30 '14 at 22:29
  • \$\begingroup\$ Updated answer now 70ms thats it \$\endgroup\$ – Heslacher Dec 30 '14 at 23:01
  • \$\begingroup\$ I think the 900ms timing is misleading. Most of that time is creating the regex object. If done properly that instantiation should only be done one. \$\endgroup\$ – Jeff Vanzella Dec 31 '14 at 3:58
  • \$\begingroup\$ @JeffVanzella As the method and the Regex are shared, the Regex is instatiated only once. \$\endgroup\$ – Heslacher Dec 31 '14 at 7:48
  • \$\begingroup\$ @Heslacher I thought that is what shared meant...not too sure VB syntax. My point still stands though, my assumption is the the regeneration is instantiated once on app start so and used the while time the app is running, so is it fair to include that in the time? :) \$\endgroup\$ – Jeff Vanzella Dec 31 '14 at 16:41
1
\$\begingroup\$

Heslacher's function with some optimizations make it about 30% faster. Use dn.IndexOf(","c) for the start of the For loop, make the last conditional a char("O"c), use Substring for the return:

Friend Function GetOrganization2(dn As String) As String
    Dim firstHit As Boolean = False
    Dim secondHit As Boolean = False
    Dim startIndex As Integer = 0
    For i As Integer = dn.IndexOf(","c) To dn.Length - 3
        If dn.Chars(i) = "="c AndAlso dn.Chars(i - 1) = "U"c AndAlso dn.Chars(i - 2) = "O"c Then
            If Not secondHit AndAlso firstHit Then
                startIndex = i + 1
                secondHit = True
                Continue For
            End If
            If secondHit Then
                Return dn.Substring(startIndex, i - startIndex - 3)
            End If
            firstHit = True
        End If
    Next
    Return String.Empty
End Function
\$\endgroup\$
  • \$\begingroup\$ Yeah Everything can be approved. \$\endgroup\$ – Yay Dec 31 '14 at 4:39
  • \$\begingroup\$ About the last condition, that had been a copy & paste error. The Substring is basically an improvement where the IndexOf() isn't. \$\endgroup\$ – Heslacher Dec 31 '14 at 8:18
  • \$\begingroup\$ I tested with both and on 5000000 iterations got improvements with each. \$\endgroup\$ – tinstaafl Dec 31 '14 at 12:22
  • \$\begingroup\$ IndexOf() can handle a future where DN can be "CN=A,OU=User,OU=Cardiff Electric,OU=Company,DC=root,DC=usa,DC=net" . This is not allowed today .. So perhaps better in that way - or not - today we get errors and handle them on the spot - which means we don't get these kind of errors ... \$\endgroup\$ – Yay Dec 31 '14 at 15:34

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.