# Program falls over with larger input files

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
'Pathing Allowance
'Performance Allowance
Dim last As Integer = cifSchedule.TrainArray.Count() - 1
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
'Pathing Allowance
'Performance Allowance

Dim last As Integer = cifSchedule.TrainArray.Count() - 1
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
workingArrival, platform))
cifSchedule.TrainArray(last).locationList.Sort()
Exit Select
End If

End Select
Next

Catch kex As KeyNotFoundException
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

-
Just to be clear, the C# code works flawlessly with larger files? –  Mat's Mug Jun 17 at 15:49
On tests with files up to ~20MB or so, which is the realistic upper limit. I should probably test it with a full 800MB monster at some point. –  Guy F-W Jun 17 at 19:06

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"}

If line IsNot Nothing AndAlso validTypes.Contains(line.Substring(0, 2)) Then
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

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)
While (True)
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.

-
Looks promising - I'm going to have a go at refactoring to only run through the input once, after I've read up on the Iterator and Yield keywords you've used (not ones I know). –  Guy F-W Jun 18 at 8:09
Accepted, because I've refactored to use a structure much like the one you suggested, and it's done the trick - tests have handled input of up to 42MB without any trouble, and I only stopped there because it was the largest test file I had handy. Thanks for your help. –  Guy F-W Jun 23 at 7:56

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.

Dim allowances As Double = 0.0
'Engineering Allowance
'Pathing Allowance
'Performance Allowance


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
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
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.

-

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.

-