Reddit Feedback

[MarkPflug]

I saw your post on reddit the other day and finally had a chance to sit down and look at your project. I was going to post this to reddit but... it exceeds the reddit response length. There also wouldn't have been much point posting to a dead thread...

I'm the author of the Sylvan "suite" of libraries and I was interested in comparing the subset of your suite that overlaps with mine: CSV and Excel.

I think the problem with any new library is that there needs to be some compelling reason for people to adopt it. There are already a lot of CSV and Excel libraries in the .NET ecosystem. Some of those libraries have effectively "won" on popularity: CsvHelper, EPPlus, ExcelDataReader, etc. So, the first question is what sets your libraries apart?

I started my Sylvan libraries as toy projects to play with some optimization ideas that I had, to push the boundary of performance. I think I accomplished that, but my libraries are also pretty minimal and don't attempt to compete on feature set. I primarily just want to get data out of files as fast as possible.

Seeing your benchmark results made me curious if there were some new optimization tricks I could learn, so I set about adding them to my benchmark suite. Here's some feedback from my attempts at adding OfficeIMO.Csv and OfficeIMO.Excel to my benchmarks. I haven't published any of these results yet but I will eventually. I'd be happy to incorporate any feedback you have before I do.

CSV

CsvDataReader

This was the class that I first, intuitively, reached for. This class has the same name as the root API in my Sylvan library, so it looked familiar and sounded like what I wanted. However, it has no public constructor or static methods, so it wasn't obvious how to interact with it. It took me a while to discover that I need to use CsvDocument. I think your documentation probably mentions this, but I usually see how far I can get by just exploring the API.

CsvDocument

This seems to be the "root" of the CSV API. It has a static method for creating CsvDataReader, which feels unintuitive/undiscoverable. Why not just put that method on CsvDataReader to assist in discoverability? However, it looks like CsvDataReader isn't the most efficient approach, and it might just be an extra layer of abstraction over CsvDocument. The "Streaming and materializing" example in your documentation shows how to stream data out of the CSV file and is what I ended up using for the benchmark. This example uses the CsvDocument.Load method as the "entry point". The name of this method sounds concerning to me; since it is essentially identical to XmlDocument.Load, which eagerly loads and validates an entire XmlDocument in memory. CsvDocument.Load sounds like it is going to do the same, which is not what I want -- I want to stream the CSV records one at a time. I found that CsvDocument.AsEnumerable returns an IEnumerable<CsvRow>, which provides the streaming approach that I want.

This was the benchmark implementation that I ended up with for the "string-only" processing:

	[Benchmark]
	public void OfficeIMO()
	{
		var opts = new CsvLoadOptions
		{
			Mode = CsvLoadMode.Stream,
			HasHeaderRow = true,
		};
		using var s = TestData.GetUtf8Stream();
		var reader = CsvDocument.Load(s, opts);
		foreach(var r in reader.AsEnumerable())
		{
			for (int i = 0; i < r.FieldCount; i++)
			{
				_ = r.Get<string>(i);
			}			
		}
	}

And the results when compared to Sylvan.Data.Csv:

Method	Mean	Error	Ratio	Allocated	Alloc Ratio
Sylvan	8.086 ms	0.0198 ms	1.00	35.72 MB	1.00
OfficeIMO	27.616 ms	0.4626 ms	3.42	109.73 MB	3.07

This doesn't match your benchmark performance claims, which could be for a few reasons. Maybe I'm "holding it wrong" and there's a more efficient API that I missed? My library has SIMD optimization for x86, but none for arm. Maybe your library is the inverse, and you're running on an apple machine, so our results are flipped? I didn't see the benchmark configuration anywhere in the docs, but maybe I missed it. Or, maybe your benchmark implementation for Sylvan is flawed, but I can't correlate your results to the actual benchmark implementation so I don't know where to look.

For the strongly-typed processing, I used this implementation:

[Benchmark]
public void OfficeIMO()
{
	var opts = new CsvLoadOptions
	{
		Mode = CsvLoadMode.Stream,
		HasHeaderRow = true,
	};
	using var s = TestData.GetUtf8Stream();
	var reader = CsvDocument.Load(s, opts);
	foreach (var r in reader.AsEnumerable())
	{

		var rec = new SalesRecord
		{
			Region = r.Get<string>(0),
			Country = r.Get<string>(1),
			ItemType = r.Get<string>(2),
			SalesChannel = r.Get<string>(3),
			OrderPriority = r.Get<string>(4),
			OrderDate = r.Get<DateTime>(5),
			OrderId = r.Get<int>(6),
			ShipDate = r.Get<DateTime>(7),
			UnitsSold = r.Get<int>(8),
			UnitPrice = r.Get<decimal>(9),
			UnitCost = r.Get<decimal>(10),
			TotalRevenue = r.Get<decimal>(11),
			TotalCost = r.Get<decimal>(12),
			TotalProfit = r.Get<decimal>(13)
		};
		
	}
}

Which produced these results:

Method	Mean	Error	Ratio	Allocated	Alloc Ratio
SylvanManual	19.54 ms	0.053 ms	1.00	22.92 MB	1.00
SylvanAuto	19.90 ms	0.071 ms	1.02	22.92 MB	1.00
OfficeIMO	55.62 ms	0.165 ms	2.85	135.73 MB	5.92

This result is comparable to CsvHelper (on my machine), which is perfectly respectable, but doesn't seem to match what your published benchmark results claim. However, the Csv read benchmark you published is named "Wide field-span CSV read", which perhaps indicates that there is a scenario where your library shines and mine struggles? I'm having a hard time correlating your benchmark results to the implementation though. Or maybe this is the same SIMD inversion that I suggested above.

In my cursory exploration of the API I didn't see any "killer feature" that would make me select this library over the competition. Of course, I'm biased toward my own libraries, but I'd expect most people would just use CsvHelper which is extremely battle hardened and certainly "fast enough".

Excel

This library appears to have a similar API to the CSV library, so ExcelDocument.Load is where I started. The same criticism of the name that it sounds like it is going to synchronously, eagerly load the entire file into memory like XmlDocument.Load. The ExcelLoadOptions has a property called "AccessMode" for readonly/readwrite... why is the DocumentAccessMode enum in the OfficeIMO.Drawing namespace? That feels misplaced. It appears that it is critical to set to readonly, as the performance is significantly slower in readwrite mode (which is the default I think?).

I then get a worksheet with the GetSheet(string name) method, there doesn't seem to be an API to access sheets ordinally, which is fine because you can use GetSheetNames to discover all the sheets. Once I have a sheet, my intuition is to use the same code pattern as I used with the CSV code, but AsEnumerable() doesn't exist. I find Sheet.Rows, which sounds like what I want, but it returns an IEnumerable<Dictionary<string,object>>. That sounds expensive: the return type implies that every cell in every row will be processed and added to a new dictionary for every row. Then I see Sheet.RowObjects(ExcelReadOptions), which returns IEnumerable<RowEdit>, which sounds more promising than Dictionary. I don't plan on editing anything so I'm still concerned I'm on the wrong path due to the RowEdit naming. I pass null for ExcelReadOptions, and hope the default behavior is what I want. RowEdit also has a Get<T> method, like the CsvRow, but it doesn't accept ordinals, only header names. Why the mismatch? Accessing by ordinal would almost certainly be faster than by name.

I end up with this implementation:

[Benchmark]
public void OfficeIMOXlsx()
{
	var opts = new ExcelLoadOptions { 
		AccessMode = OfficeIMO.Drawing.DocumentAccessMode.ReadOnly 
	};
	using var reader = ExcelDocument.Load(file, opts);

	foreach(var name in reader.GetSheetNames())
	{
		var sheet = reader.GetSheet(name);

		foreach (var r in sheet.RowsObjects())
		{
			var rec = new SalesRecord
			{
				Region = r.Get<string>("Region"),
				Country = r.Get<string>("Country"),
				ItemType = r.Get<string>("Item Type"),
				SalesChannel = r.Get<string>("Sales Channel"),
				OrderPriority = r.Get<string>("Order Priority"),
				OrderDate = r.Get<DateTime>("Order Date"),
				OrderId = r.Get<int>("Order Id"),
				ShipDate = r.Get<DateTime>("Ship Date"),
				UnitsSold = r.Get<int>("Units Sold"),
				UnitPrice = r.Get<decimal>("Unit Price"),
				UnitCost = r.Get<decimal>("Unit Cost"),
				TotalRevenue = r.Get<decimal>("Total Revenue"),
				TotalCost = r.Get<decimal>("Total Cost"),
				TotalProfit = r.Get<decimal>("Total Profit")
			};
		}
	}

}

This benchmark implementation produced these results:

Method	Mean	Error	Ratio	Allocated	Alloc Ratio
BaselineXml	117.9 ms	0.24 ms	1.00	246.58 KB	1.00
SylvanXlsx	164.0 ms	0.38 ms	1.39	665.77 KB	2.70
OfficeIMOXlsx	304.0 ms	1.68 ms	2.58	150298.09 KB	609.54

That is a very respectable result, faster than the majority of libraries, but also not as fast as your results would suggest. This is just one benchmark though, so there may very well be scenarios where your library is faster. The "Baseline" here is a benchmark that unzips and uses XmlReader.Read to process the main sheet in the same file. It is meant to show a performance "floor". It is possible to go faster than that, but would require using a different zip and/or XML implementation.

I also added a benchmark for your "object binder" feature.

	[Benchmark]
	public void OfficeIMOXlsx()
	{
		var opts = new ExcelLoadOptions
		{
			AccessMode = OfficeIMO.Drawing.DocumentAccessMode.ReadOnly
		};
		using var reader = ExcelDocument.Load(file, opts);

		foreach (var name in reader.GetSheetNames())
		{
			var sheet = reader.GetSheet(name);

			foreach (var r in sheet.RowsAs<SalesRecord>("A1:N65536"))
			{
				;
			}
		}
	}

My main criticism here, is the RowsAs<T> method requires you pass an Excel range specifier. I can't imagine a scenario where I'd necessarily know the range that I want to process ahead of time. I'd typically want to process all of the populated rows in the sheet, but I can't see an obvious way to do that.

Method	Mean	Error	Ratio	Allocated	Alloc Ratio
BaselineXml	117.9 ms	0.15 ms	1.00	246.6 KB	1.00
SylvanXlsx	170.1 ms	0.68 ms	1.44	10906.89 KB	44.23
OfficeIMOXlsx	277.1 ms	1.45 ms	2.35	125668.08 KB	509.61

Somewhat surprisingly, this is even faster and more memory efficient than the previous code that was only accessing the values manually, and not allocating SalesRecord instances. I must be missing something to speed up the previous benchmark, but I don't know what.

Your library also doesn't seem to handle decimal values "correctly" (in agreement with Excel). As an example, a value in Excel of "165258.24" will be read as a decimal value "165258.23999999999". In my experimenting I've found that the majority of libraries have this incorrect behavior. Reading as double, then casting to decimal will produce the expected value, but I don't think most users will know to do that.

XLSB

The exact same code for the xlsx benchmark can be used for xlsb files, which I think is great. Not all libraries do that, some expose distinct classes to support the different file types.

However, the benchmark results are pretty disappointing:

Method	Mean	Error	Ratio	Allocated	Alloc Ratio
SylvanXlsb	25.38 ms	0.024 ms	1.00	358.95 KB	1.00
OfficeIMOXlsb	4,573.38 ms	82.844 ms	180.20	2179501.43 KB	6,071.84

Final thoughts

I think a lot of people see CSV and .xlsx (and xls/xlsb) as being "the same", so having the API to work with them (at least to read them) be identical or as close as possible is a worthy goal. Your CSV library doesn't have a "RowsAs" similar to the Excel library. I would ideally want to only have to learn one API that can be used for both formats. I think there's room for improvement here.

Given all that, I don't know why anyone would choose to use these libraries. I'm not going to investigate or comment on the rest of the suite because I don't have any particular expertise or interest in them.

Hopefully you find this feedback useful.

[PrzemyslawKlys]

Hello @MarkPflug, please leave this open if possible. It's very valuable feedback that I would like to address. But before I do I decided to go back to drawing board and make sure I fix some of the issues you have pointed out, so potentially you could asses it again with your expert eyes. While I wanted to write right away I think it makes more sense to first fix, release and give you all in one response.