5
\$\begingroup\$

I have a spreed sheet and its something like 550+ columns and I need to pull URLs from it. Previously I was using LinqToExcel when the sheet was less than 255 columns a recent update has produced this monstrosity, so I have moved on to interop with excel to parse the data.

Needless to say I have a list of a string I want to name all the images to, plus the index of that image for this item(uniqueness of file names). Currently I use 10 threads to do the queries for the data. 10 HttpClients, 1 on each thread, and then I use a paralle.ForEach and Partitioner to dole out batches of 100 items to each parallel thread. This is not a Image pull from 1 website, many websites may be covered in the items.

Is This the Fastest way to do it? Old Code

Relevant new code:

Imports System.Collections
Imports System.Collections.Generic
Imports System.Collections.Concurrent
Imports System.Data
Imports System.Linq
Imports System.Net
Imports System.Net.Http
Imports System.IO
Imports System.Text
Imports System.Xml.Linq
Imports Microsoft.Office.Interop.Excel
Imports System.Threading.Tasks


Public Class MDI
Public Class ImgThreadParams
    Public gtin As String
    Public lUrls As List(Of String) = New List(Of String)()
End Class
Public Shared Function GetXlsxFiles(targetFolder As String) As String()
    Dim files As String()
    If targetFolder = vbNullString Then
        files = System.IO.Directory.GetFiles(System.IO.Directory.GetCurrentDirectory(), "Export*.xlsx")
    Else
        If System.IO.Directory.Exists(targetFolder) = True Then
            files = System.IO.Directory.GetFiles(targetFolder, "Export*.xls")
        Else
            files = System.IO.Directory.GetFiles(System.IO.Directory.GetCurrentDirectory(), "Export*.xlsx")
        End If
    End If
    Return files
End Function
Public Shared Sub ProcessFolder(targetFolder As String)
    Dim ListParams As New List(Of ImgThreadParams)()
    Dim gtin As String = vbNullString
    Dim files = GetXlsxFiles(targetFolder)
    Dim excel As Application = New Application()
    Try

        For Each fileName As String In files
            'Dim book = New LinqToExcel.ExcelQueryFactory(System.IO.Path.GetFileName(fileName))
            Dim w As Workbook = excel.Workbooks.Open(fileName)
            ' Get sheet.
            Dim sheet As Worksheet = CType(w.Sheets(1), Worksheet)
            ' Get range.
            Dim r As Range = sheet.UsedRange()
            Dim rArray(,) As Object = CType(r.Value(XlRangeValueDataType.xlRangeValueDefault), Object(,))
            Dim hl As Integer = 0
            Dim hn As Integer = 0
            Dim rt As Integer = 0
            Dim GtinC As Integer = 0
            Dim Img1 As Integer = 0
            Dim Img2 As Integer = 0
            Dim Img3 As Integer = 0
            Dim Img4 As Integer = 0
            Dim Img5 As Integer = 0
            Dim Img6 As Integer = 0
            Dim Img7 As Integer = 0
            Dim Img8 As Integer = 0
            Dim Img9 As Integer = 0
            Dim Img10 As Integer = 0
            Dim ic As Integer = 0
            Dim ic3 As Integer = 0
            For j As Integer = 1 To r.Columns.Count()

                If rArray(4, j).ToString().StartsWith("GTIN") Then
                    GtinC = j
                ElseIf rArray(4, j).Equals("Record type") Then
                    rt = j
                ElseIf rArray(4, j).ToString.Equals("Hierarchy number") Or rArray(4, j).ToString.Equals("Hierarchy level") Then
                    ic3 = ic3 + 1
                    If (ic3 = 1) Then
                        hn = j
                    ElseIf ic3 = 2 Then
                        hl = j
                    End If
                ElseIf rArray(4, j).ToString.StartsWith("Image") And Not rArray(4, j).ToString.EndsWith("Description") And Not rArray(4, j).ToString.Contains("File") Then
                    ic = ic + 1
                    Select Case ic
                        Case 1
                            Img1 = j
                        Case 2
                            Img2 = j
                        Case 3
                            Img3 = j
                        Case 4
                            Img4 = j
                        Case 5
                            Img5 = j
                        Case 6
                            Img6 = j
                        Case 7
                            Img7 = j
                        Case 8
                            Img8 = j
                        Case 9
                            Img9 = j
                        Case 10
                            Img10 = j
                        Case Else
                            Exit Select
                    End Select
                End If
            Next
            Dim additem As Boolean = False
            For i As Integer = 5 To r.Rows.Count()
                'check Hierarchy Number Level and record Type to filter for only top level GTIN's to download
                If (CType(rArray(i, hn), Integer) = 1 And CType(rArray(i, hl), Integer) = 1 And CType(rArray(i, rt), Integer) = 1) Then
                    GoTo ContinueLoop
                Else
                    Continue For
                End If

ContinueLoop:
                Dim itp As ImgThreadParams = New ImgThreadParams()
                itp.gtin = CType(rArray(i, GtinC), String)
                If (rArray(i, Img1) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img1), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img1), String))
                        additem = True
                    End If
                End If
                If (rArray(i, Img2) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img2), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img2), String))
                    End If
                End If
                If (rArray(i, Img3) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img3), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img3), String))
                    End If
                End If
                If (rArray(i, Img4) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img4), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img4), String))
                    End If
                End If
                If (rArray(i, Img5) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img5), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img5), String))
                    End If
                End If
                If (rArray(i, Img6) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img6), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img6), String))
                    End If
                End If
                If (rArray(i, Img7) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img7), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img7), String))
                    End If
                End If
                If (rArray(i, Img8) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img8), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img8), String))
                    End If
                End If
                If (rArray(i, Img9) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img9), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img9), String))
                    End If
                End If
                If (rArray(i, Img10) IsNot Nothing) Then
                    If (Uri.IsWellFormedUriString(CType(rArray(i, Img10), String), UriKind.Absolute)) Then
                        itp.lUrls.Add(CType(rArray(i, Img10), String))
                    End If
                End If
                If (additem = True) Then
                    ListParams.Add(itp)
                    additem = False
                End If
            Next
            rArray = Nothing
            w.Close()
            Runtime.InteropServices.Marshal.ReleaseComObject(r)
            Runtime.InteropServices.Marshal.ReleaseComObject(sheet)
            Runtime.InteropServices.Marshal.ReleaseComObject(w)
            Runtime.InteropServices.Marshal.ReleaseComObject(excel)


            Dim Folder As DirectoryInfo = System.IO.Directory.CreateDirectory(System.IO.Directory.GetCurrentDirectory() + "\Image")
            If ListParams.Count > 10 Then
                Dim tCount As Integer = CInt(Math.Round(ListParams.Count / 10, MidpointRounding.ToEven))
                Dim L1 As New List(Of ImgThreadParams)
                For i As Integer = 0 To tCount
                    L1.Add(ListParams(i))
                Next
                Dim L2 As New List(Of ImgThreadParams)
                For i As Integer = tCount + 1 To tCount * 2
                    L2.Add(ListParams(i))
                Next
                Dim L3 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 2 + 1 To tCount * 3
                    L3.Add(ListParams(i))
                Next
                Dim L4 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 3 + 1 To tCount * 4
                    L4.Add(ListParams(i))
                Next
                Dim L5 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 4 + 1 To tCount * 5
                    L5.Add(ListParams(i))
                Next
                Dim L6 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 5 + 1 To tCount * 6
                    L6.Add(ListParams(i))
                Next
                Dim L7 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 6 + 1 To tCount * 7
                    L7.Add(ListParams(i))
                Next
                Dim L8 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 7 + 1 To tCount * 8
                    L8.Add(ListParams(i))
                Next
                Dim L9 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 8 + 1 To tCount * 9
                    L9.Add(ListParams(i))
                Next
                Dim L10 As New List(Of ImgThreadParams)
                For i As Integer = tCount * 9 + 1 To ListParams.Count - 1
                    L10.Add(ListParams(i))
                Next
                Threading.ThreadPool.SetMaxThreads(1000, 1000)
                Dim t1, t2, t3, t4, t5, t6, t7, t8, t9, t10 As Threading.ParameterizedThreadStart
                t1 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t2 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t3 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t4 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t5 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t6 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t7 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t8 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t9 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t10 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t1.Invoke(L1)
                t2.Invoke(L2)
                t3.Invoke(L3)
                t4.Invoke(L4)
                t5.Invoke(L5)
                t6.Invoke(L6)
                t7.Invoke(L7)
                t8.Invoke(L8)
                t9.Invoke(L9)
                t10.Invoke(L10)
            Else
                Dim t1 As Threading.ParameterizedThreadStart
                t1 = New Threading.ParameterizedThreadStart(AddressOf ThreadedGetGtinImages)
                t1.Invoke(ListParams)
            End If
        Next
    Catch ex As Exception
        Exit Sub
    End Try
End Sub
Public Shared Sub ThreadedGetGtinImages(Params As List(Of ImgThreadParams))

    Dim wc As HttpClient = New HttpClient()
    Const ItemBatch As Integer = 100
    Parallel.ForEach(Partitioner.Create(0, Params.Count, ItemBatch), Async Sub(range)
                                                                         Dim imgCount As Integer = 1
                                                                         For i As Integer = range.Item1 To range.Item2 - 1
                                                                             Dim item = Params(i)
                                                                             imgCount = 1
                                                                             For Each surl In item.lUrls
                                                                                 Try
                                                                                     Dim response As HttpResponseMessage = Await wc.GetAsync(surl)
                                                                                     If (response.StatusCode = 200) Then
                                                                                         Dim sContentType As String = response.Content.Headers.ContentType.MediaType.ToString()
                                                                                         If (sContentType.Length > 0) Then
                                                                                             Dim sImage As String = sContentType.Split("/")(0).ToLower
                                                                                             Dim sImageType As String = sContentType.Split("/")(1).ToLower
                                                                                             If sImage.Equals("image") = True Or sImage.Equals("text") Then
                                                                                                 Dim sFileExtension As String
                                                                                                 Select Case sImageType
                                                                                                     Case "jpeg"
                                                                                                         sFileExtension = ".jpg"
                                                                                                     Case "png"
                                                                                                         sFileExtension = ".png"
                                                                                                     Case "bmp"
                                                                                                         sFileExtension = ".bmp"
                                                                                                     Case "gif"
                                                                                                         sFileExtension = ".gif"
                                                                                                     Case "tiff"
                                                                                                         sFileExtension = ".Tiff"
                                                                                                     Case "html"
                                                                                                         sFileExtension = ".html"
                                                                                                     Case Else
                                                                                                         Continue For
                                                                                                 End Select
                                                                                                 Dim sFolder = Directory.GetCurrentDirectory() + "\Image"
                                                                                                 Using Fi As FileStream = New FileStream(sFolder + "\" + item.gtin + "_" + imgCount.ToString + sFileExtension, FileMode.OpenOrCreate)
                                                                                                     Dim d = Await response.Content.ReadAsByteArrayAsync()
                                                                                                     Await Fi.WriteAsync(d, 0, d.Length)
                                                                                                     Await Fi.FlushAsync()
                                                                                                 End Using
                                                                                                 imgCount = imgCount + 1
                                                                                             End If
                                                                                         End If
                                                                                     End If
                                                                                 Catch
                                                                                     Continue For
                                                                                 End Try
                                                                             Next
                                                                         Next
                                                                     End Sub)


End Sub
Public Shared Sub Main()
    ServicePointManager.DefaultConnectionLimit = 10000
    ProcessFolder(System.IO.Directory.GetCurrentDirectory())
    Console.ReadLine()
End Sub

End Class
\$\endgroup\$
  • 1
    \$\begingroup\$ My initial thought is that excel at 550+ columns, it's a good sign you're probably using the wrong tool for the job. Can you tell us: where is your code slow? What is the bottleneck (memory, disk, network?) ...or is that the part you don't know how to determine? \$\endgroup\$ – James Snell Sep 15 '16 at 16:25
  • \$\begingroup\$ Just going to throw this link: Using Task.WhenAll That should hopefully help to understand how to use the async API properly. \$\endgroup\$ – Ext3h Sep 15 '16 at 17:13
  • \$\begingroup\$ @james Snell: well the code is not slow it gets over 2k images in about 10 mins, but I want it to run as fast as possible given any machine that runs it. \$\endgroup\$ – BanMe Sep 15 '16 at 17:31
  • \$\begingroup\$ @Ext3h there is to much overhead created by tasks. my last implemention used Task.WhenAll. :) Thank you though. \$\endgroup\$ – BanMe Sep 15 '16 at 17:56
  • 1
    \$\begingroup\$ Mere 2k images, and the overhead from using tasks was already too much? Find that hard to believe. Besides, your previous solution had the same mess with manually defining a "threadpool". Strip that manual threading, keep the async implementation from your second attempt (instead of Task.Factory.StartNew wrapping synchronous code!), go back to only a single image per task. There is simply no way you are CPU bound during download, so don't even bother to thread. Going async alone is sufficient. \$\endgroup\$ – Ext3h Sep 15 '16 at 18:34
9
\$\begingroup\$

Just a few quick remarks (for now):

  • vbNullString is a remnant of VB6 (which I think would be under the Microsoft.VisualBasic.Constants namespace), and used to be a null string pointer often confused with "", an empty string. In VB.NET vbNullString is Nothing, so when you do this:

    If targetFolder = vbNullString Then
    

    You're actually verifying whether targetFolder contains a reference at all... but you probably mean to check whether the provided string is null or empty. A more idiomatic way to test for this is String.IsNullOrEmpty:

    If String.IsNullOrEmpty(targetFolder) Then
    

    VB.NET's String type overrides the = operator such as Not String.IsNullOrEmpty(s), s <> "", s <> String.Empty and s <> vbNullString are all equivalent.

  • The following only apply if your project defaults options aren't customized:

    • You're not specifying Option Explicit explicitly, but that's fine because in VB.NET that's the default behavior, contrary to VB6/VBA where implicit declarations are allowed by default.
    • You're not specifying Option Strict either, and that's less fine, because by default, the VB.NET compiler won't enforce strict data typing - VB6/VBA was very loose with type conversions, and without Option Strict, VB.NET keeps a lot of that looseness. Note that Option Strict implies Option Explicit, so you don't have to specify both.
  • You're specifying Imports statements, but then you fully-qualify things in these imported namespaces - this defeats the purpose: either you Imports namespaces, or you fully-qualify stuff. For example, you have Imports System.IO, so this code:

    files = System.IO.Directory.GetFiles(System.IO.Directory.GetCurrentDirectory(), "Export*.xlsx")
    

    Could read like this instead:

    files = Directory.GetFiles(Directory.GetCurrentDirectory(), "Export*.xlsx")
    

There really isn't much of a reason to directly work with a Thread anymore, especially given Imports System.Threading.Tasks. Let the framework deal with the metal-level threading stuff: it's very unlikely that starting 10 threads is buying you anything in reality - no computer has 10 CPU cores anyway.

It's impossible to read ProcessFolder and know what's going on at a glance. The procedure is doing WAY too many things, at a WAY too low abstraction level. Extract methods out of code blocks, reduce the nesting: this is a clear sign that something isn't right:

OP's code, so far indented to the right that almost no code is even visible

That ThreadedGetGtinImages procedure is running on a thread already - and then you start a Parallel.ForEach loop inside that to start an Async Sub that does its thing... Get rid of the threads, and launch asynchronous tasks instead: I suspect your code has a TON of threading overhead that's nullifying the benefits of it all.

Also... whenever you feel the need to name something t1, t2, t3, ..., t12 ...it means you've missed an opportunity to use an appropriate data structure, e.g. some array or some List(Of Something) - that would have eliminated all that huge chunk of repeated code there, where you populate the Ln lists - IMO that should be a function in its own right.

The identifiers you choose aren't consistently meaningful - I get ProcessFolder and excel and sheet, but t7 and itp and r and w and hn and hlare terrible names that will definitely make you want to pull your hair off if you drop this and come back to it in 6 months from now. By the way hl is pure evil, especially in a code base where it might just as well be h1.

\$\endgroup\$
  • \$\begingroup\$ I will take all of what you say under advisement and update to the code to use more meaningful names for variables.I have reworked this code many different ways before, but this method seems to accomplish the goal the fastest so far. I will look at what I can do to reduces the nested complexity of it. \$\endgroup\$ – BanMe Sep 15 '16 at 15:15
  • \$\begingroup\$ You say, "it's very unlikely that starting 10 threads is buying you anything in reality - no computer has 10 CPU cores anyway"; but for networking stuff, where connection handshakes and packet retries can take entire milliseconds and are very much network- rather than CPU-bound, concurrency can help a lot even on single processor machines. I can't read VB very well, though; so it's possible this is not what OP is using the threads for, in which case I apologize for my useless tangent. \$\endgroup\$ – Daniel Wagner Sep 15 '16 at 19:09
  • \$\begingroup\$ @DanielWagner it's a valid point... to a certain extent - non-CPU bound work should be asynchronous, not threaded: OP doesn't need threads, he needs tasks - and let the framework determine if a task would be more efficiently executed on its own thread or not. \$\endgroup\$ – Mathieu Guindon Sep 15 '16 at 19:36
  • \$\begingroup\$ So i have turn it to all tasks and did a whenall and I get numerous TaskCancelationExceptions as well HttpRequestException. Is there a task Limit!? or request limit that I am hitting? i am doing 1786 concurrent calls it works some of the time but fails alot.. \$\endgroup\$ – BanMe Sep 16 '16 at 4:55
  • \$\begingroup\$ You're doing something wrong, if you can reproduce the issue with minimal code that will make a good SO question. \$\endgroup\$ – Mathieu Guindon Sep 16 '16 at 11:19

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.