New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

C# improvements #6

Open

tedd wants to merge 6 commits into losvedir:main from tedd:main

tedd commented Oct 23, 2022

Huge PR, so here is some info:

Had a look at the C# version, and there were some obvious issues. So I forked, set up a benchmark project and added progressive improvements to it - since its fun to benchmark things. See TrannetVersions

Benchmark results: https://github.com/tedd/transit-lang-cmp/tree/main/Trannet.Benchmark
Also added changes + simplified webapi implementation here: https://github.com/tedd/transit-lang-cmp/tree/main/Trannet

tedd added 4 commits

October 22, 2022 23:25


          Benchmark and improvements

fa28dac


          Updated results

fb8a7f5


          CSV library + simplified web api

6be0799


          Remove obj folder

4ae61b1

jeremylcarter commented Oct 23, 2022

Great job!

tedd added 2 commits

October 23, 2022 22:37


          TripResponse needs properties for default json behavior

311e51c


          Larger json buffer + include fields

a63c651

MarkPflug commented Oct 24, 2022

Good choice of CSV library. I approve.

MarkPflug reviewed

View reviewed changes

MarkPflug left a comment

Added a few suggestions. The list/dictionary capacity might have a measurable impact on the benchmarks.

Trannet.Benchmark/TrannetVersions/_04_CsvReader/GTFS.cs

+                      var filename = Path.Join(RootDir, "MBTA_GTFS", "/trips.txt");
+                      var opts = new CsvDataReaderOptions { HasHeaders = true, Delimiter = ',', Culture = new System.Globalization.CultureInfo("en-US") };
+                      var csv = CsvDataReader.Create(filename, opts);
+                      csv.Read();

MarkPflug Oct 24, 2022

This Read should be removed. When HasHeaders = true, the header row will already be consumed when Create is called. The Culture doesn't need to be set, it defaults to Invariant, which is usually what you want.

Trannet.Benchmark/TrannetVersions/_04_CsvReader/GTFS.cs

+                  public static (List<StopTime>, Dictionary<string, List<int>>) LoadStopTimes()
+                  {
+                      var stopTimes = new List<StopTime>();

MarkPflug Oct 24, 2022

Specifying an initial capacity on the List can avoid growing/reallocating the internal buffer.

Trannet.Benchmark/TrannetVersions/_04_CsvReader/GTFS.cs

+                  public static (List<StopTime>, Dictionary<string, List<int>>) LoadStopTimes()
+                  {
+                      var stopTimes = new List<StopTime>();
+                      var stopTimesIxByTrip = new Dictionary<String, List<int>>();

MarkPflug Oct 24, 2022

Same is true for this dictionary.

Trannet.Benchmark/TrannetVersions/_04_CsvReader/GTFS.cs

+                      var stopTimesIxByTrip = new Dictionary<String, List<int>>();
+                      var filename = Path.Join(RootDir, "MBTA_GTFS", "/stop_times.txt");
+                      var opts = new CsvDataReaderOptions { HasHeaders = true, Delimiter = ',', Culture = new System.Globalization.CultureInfo("en-US") };
+                      var csv = CsvDataReader.Create(filename, opts);

MarkPflug Oct 24, 2022

using var csv =... to ensure things get closed if exceptions happen.

Trannet.Benchmark/TrannetVersions/_04_CsvReader/GTFS.cs

+                              stopTimesIxByTrip.Add(tripID, new List<int> { stopTimes.Count - 1 });
+                          }
+                      }
+                      csv.Close();

MarkPflug Oct 24, 2022

Close is not needed when using is added above.

MarkPflug commented Oct 24, 2022 •

edited

Loading

~~Loading the stop_times.txt file also benefits from enabling string pooling.~~ The data in that file is quite repetitive, and the string pooling allows only loading each unique string once. A quick test on my machine shows ~40% reduction in time to load the file. The StringPool comes from the Sylvan.Common nuget package.

var pool = new Sylvan.StringPool();

var opts = new CsvDataReaderOptions { 
    StringFactory = pool.GetString 
};

UPDATE: after some more experimentation I found this to be wrong. In ASP.NET, under the server GC, the difference, if any, is negligible. My original measurements were in a console app (non-server GC), where the performance difference was significant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet