Program falls over with larger input files

Question

I have been translating a solution from C# into VB.Net, because I know more VB and wanted to understand it better. I have a version up and running that replicates the C# performance with small input files; however with an input file over ~8MB the memory usage skyrockets and it will keep chugging until it consumes all memory. This is a problem, because the expected use case includes input files over 10MB, up to maybe 20MB.

I've run the code through the Visual Studio profiler on smaller files, and the below function is doing almost all the gruntwork (80%ish). The rest of the class is on paste-bin here if it helps.

The key question, am I doing anything obviously idiotic, that scales really badly, or is substantially different between VB.Net and C#? (C# code, for comparison, is here)

    Public Function BuildSchedule() As Schedule
    Dim cifSchedule = New Schedule()
    Try
        Dim tiploc_1 As Tiploc = Tiploc.GetInstance()
        For i As Integer = 0 To _filelines.Count - 1
            Dim stopLocation As String
            Select Case _filelines(i).Substring(0, 2)
                Case "BS"
                    If True Then
                        Dim passStops As String() = {"1", "2"}
                        If passStops.Contains(_filelines(i).Substring(32, 1)) = False OrElse _filelines(i).Substring(2, 1) = "D" Then
                            Dim [stop] As Integer = _filelines.Count()
                            Dim newi As Integer = 0
                            For j As Integer = i + 1 To [stop] - 1
                                If _filelines(j).Substring(0, 2) = "BS" Then
                                    [stop] = j
                                    newi = j - 1
                                End If
                            Next

                            i = newi
                            Exit Select
                        End If
                        BuildTrain(cifSchedule, i)
                        Exit Select
                    End If

                Case "LO"
                    If True Then
                        Const stopType As String = "Origin"
                        stopLocation = _filelines(i).Substring(2, 7).Trim()
                        stopLocation = tiploc_1.getFullName(stopLocation)
                        Dim publicDeparture As DateTime = DateTime.ParseExact(_filelines(i).Substring(15, 4), "HHmm", CultureInfo.InvariantCulture)
                        Dim workingDeparture As DateTime = workingTimes(i, 10)
                        Dim platform As String = _filelines(i).Substring(19, 3).Trim()
                        Dim line As String = _filelines(i).Substring(22, 3).Trim()
                        Dim allowances As Double = 0.0
                        'Engineering Allowance
                        allowances = allowancesAdd(i, 25)
                        'Pathing Allowance
                        allowances = allowancesAdd(i, 27)
                        'Performance Allowance
                        allowances = allowancesAdd(i, 41)
                        Dim last As Integer = cifSchedule.TrainArray.Count() - 1
                        cifSchedule.TrainArray(last).AddLocation(New Location(stopType, stopLocation,
                                                                              publicDeparture, workingDeparture,
                                                                              platform, line, allowances))
                        Exit Select
                    End If

                Case "LI"
                    If True Then
                        Dim validStops As String() = {"T", "U", "D"}
                        If validStops.Contains(_filelines(i).Substring(42, 1)) = False Then
                        Else
                            Const stopType As String = "Intermediate"
                            stopLocation = _filelines(i).Substring(2, 7).Trim
                            stopLocation = tiploc_1.getFullName(stopLocation)
                            Dim publicArrival As DateTime = DateTime.ParseExact(_filelines(i).Substring(25, 4), "HHmm", CultureInfo.InvariantCulture)
                            Dim publicDeparture As DateTime = DateTime.ParseExact(_filelines(i).Substring(29, 4), "HHmm", CultureInfo.InvariantCulture)
                            'Flag for inputting Half-minutes to working times, if possible.
                            Dim workingArrival As DateTime = workingTimes(i, 10)
                            Dim workingDeparture As DateTime = workingTimes(i, 15)
                            Dim platform As String = _filelines(i).Substring(33, 3).Trim
                            Dim line As String = _filelines(i).Substring(36, 3).Trim

                            Dim allowances As Double = 0.0
                            'Engineering Allowance
                            allowances = allowancesAdd(i, 54)
                            'Pathing Allowance
                            allowances = allowancesAdd(i, 56)
                            'Performance Allowance
                            allowances = allowancesAdd(i, 58)

                            Dim last As Integer = cifSchedule.TrainArray.Count() - 1
                            cifSchedule.TrainArray(last).AddLocation(New Location(stopType, stopLocation, publicArrival,
                                                                                  publicDeparture, workingArrival,
                                                                                  workingDeparture, platform, line,
                                                                                  allowances))

                        End If
                        Exit Select
                    End If

                Case "LT"
                    If True Then
                        Const stopType As String = "Terminus"
                        stopLocation = _filelines(i).Substring(2, 7).Trim
                        stopLocation = tiploc_1.getFullName(stopLocation)
                        Dim publicArrival As DateTime = DateTime.ParseExact(_filelines(i).Substring(15, 4), "HHmm", CultureInfo.InvariantCulture)
                        Dim workingArrival As DateTime = workingTimes(i, 10)
                        Dim platform As String = _filelines(i).Substring(19, 3)

                        Dim last As Integer = cifSchedule.TrainArray.Count() - 1
                        cifSchedule.TrainArray(last).AddLocation(New Location(stopType, stopLocation, publicArrival,
                                                                              workingArrival, platform))
                        cifSchedule.TrainArray(last).locationList.Sort()
                        Exit Select
                    End If


            End Select
        Next

    Catch kex As KeyNotFoundException
        Throw New LoadException(kex.Message)
    Catch ex As Exception
        Throw New LoadException("Error: " & ex.Message & Environment.NewLine & "Please ensure CIF is not corrupt")
    End Try
    If cifSchedule.TrainArray.Count = 0 Then
        Throw New LoadException("Error: Invalid CIF" & Environment.NewLine & "No trains found")
    End If

    cifSchedule.TrainArray.Sort()
    Return cifSchedule
End Function

Answer 1

The first thing that strikes me about the code (linked not present) is that the path is passed into the constructor and all the lines read - checking them for validity - and then we make a second call (BuildSchedule()) to process them. Which means we need to pass through our large file twice. I am also a bit dubious of the end-of-file check being used.

Class CifData
    Private ReadOnly _filelines As List(Of String)
    Public Sub New(filepath As String)
        _filelines = New List(Of String)()

        Try
            Dim validTypes As String() = {"BS", "BX", "LO", "LI", "LT"}

            Using CIFReader = New StreamReader(filepath)
                While CIFReader.Peek() <> -1
                    Dim line As String = CIFReader.ReadLine()

                    If line IsNot Nothing AndAlso validTypes.Contains(line.Substring(0, 2)) Then
                        _filelines.Add(line)
                    End If

                End While
            End Using
        Catch ex As Exception
            Throw New LoadException("Error: " & ex.Message & Environment.NewLine &
                                    "Please ensure a valid CIF is selected")
        End Try
    End Sub

Is the input file a text file with a single record on each line?
If so, we can simply read until line is null. Why the Peek?

That however is small, the large problem is reading all the lines and then processing all the lines. Is there a specific reason why the lines are read in the constructor and then processed in BuildSchedule() rather than reading them in BuildSchedule()?

As well as processing the file twice we also store the information twice, once as a string and then once as a schedule entry. There seems to be no need to store the string. Perhaps this shape might help.

Public Class CifData

    Public Function BuildSchedule(fileName As String) As Schedule

        Dim ret As New Schedule

        For Each line In ReadAllLines(fileName)
            ProcessLine(line, ret)
        Next

        Return ret

    End Function

    Private Sub ProcessLine(line As String, sch As Schedule)
        ' Add to schedule based on codes.
        ' ignore lines with invalid codes
    End Sub


    Public Iterator Function ReadAllLines(fileName As String) As IEnumerable(Of String)
        Using reader = New StreamReader(fileName)
            While (True)
                Dim line As String = reader.ReadLine()
                If line Is Nothing Then
                    Exit While
                End If
                Yield line
            End While
        End Using
    End Function
End Class

It should use less memory - we are not storing the strings from the file, just using them one at a time - and less time - we only iterate through the file once.

Answer 2

This might not be the answer you're looking for, but I'm going to elaborate on @Mat'sMug's answer a bit.

1. If True Then is "useless clutter" because you will always end up
   inside that code block. It always evaluates to True. You can safely remove it.
2. You are doing too many things with your BuildSchedule function. There are definitely some opportunities to DRY this up a bit.

Let's start with this code block.

Dim allowances As Double = 0.0
'Engineering Allowance
allowances = allowancesAdd(i, 25)
'Pathing Allowance
allowances = allowancesAdd(i, 27)
'Performance Allowance
allowances = allowancesAdd(i, 41)

You take the same action in almost every case, only with different parameters. You should create a function that takes four parameters in and returns a double.

Private Function AddAllAllowances(row as integer, engineeringColumn as double, pathingColumn as double, performanceColumn) As Double
    Dim allowances As Double
    allowances = allowancesAdd(row,engineeringColumn)
    allowances = allowancesAdd(row,pathingColumn)
    allowances = allowancesAdd(row,performanceColumn)
    Return allowances
End Function

Which reminds me of three things.

• I don't see a reason to initialize the allowances variable to a value of 0.0.
• Functions/Subs should have Verb-Noun names. So, AddAllowances instead of allowancesAdd.

• Why even assign the output of the function to the variable in every case? You either only care about the Performance allowance, or you don't care about either. Assuming you only care about the Performance allowance, you end up with this function instead.

  Private Function AddAllAllowances(row as integer, engineeringColumn as double, pathingColumn as double, performanceColumn) As Double
      Dim allowances As Double
      allowancesAdd(row,engineeringColumn)
      allowancesAdd(row,pathingColumn)
      Return allowancesAdd(row,performanceColumn)
  End Function
  

There are also lot of magic numbers in your code. Here's one example:

    stopLocation = _filelines(i).Substring(2, 7).Trim()

What do 2 and 7 mean beyond a position in the string? What are those positions? I would accept these magic numbers if you had a function that returns stop location.

Private function GetStopLocation(i as Integer) As String
    Return _filelines(i).Substring(2,7).Trim
End Function 

But I do not like the ones from the ones from the "allowances" block of code at all. What data does column 25 represent? What about 27? Or 58 or 56? You should use some constants there to help Mr. Maintainer (and us) understand your code. The same could be said of your magic strings ("BS","LO","D","LI","HHmm", etc.).

I think that if you dealt with those readability issues, we would be better able to understand your code and focus on your algorithm.

Answer 3

I haven't looked at the C# code, just a few quick remarks here:

• BuildSchedule is doing too many things. I'd probably try to extract a method out of each Case block.
• If True Then is useless clutter; don't even think, remove it.

• You're swallowing all exceptions and replacing it with a LoadException:

  Catch ex As Exception
      Throw New LoadException("Error: " & ex.Message & Environment.NewLine & "Please ensure CIF is not corrupt")
  

  You're losing your stack trace, making it harder to debug. And the message looks like it belongs in a more specific Catch block than one that catches System.Exception.

I would give LoadException a constructor that takes a System.Exception parameter too, instead of just taking a String message, so that LoadException.InnerException would have the actual exception that was thrown, with its precious stack trace.