2
\$\begingroup\$

I have a VSTO Excel Add-In which is used to retrieve data from another application and insert it into an Excel worksheet. Due to the potential for lots of pieces of data to be inserted into a spreadsheet, performance is a concern. Potentially thousands of rows could be inserted.

Here is the method which inserts data into an Excel spreadsheet.

Note: Using Microsoft.Office.Interop.Excel

private Worksheet Worksheet { get { return Globals.ThisAddIn.Application.ActiveSheet; } }
private Range Cells { get { return Globals.ThisAddIn.Application.ActiveSheet.Cells; } }

private void PopulateCurrentWorksheet(ReportData data, int columnIndex, int rowIndex)
{
    if (data == null)
        throw new ArgumentNullException("No valid ReportData available");

    // NOTE: Excel uses indexing that starts at 1
    if (columnIndex == 0)
        columnIndex = 1;

    if (rowIndex == 0)
        rowIndex = 1;

    int startingRowIndex = rowIndex;

    foreach (ReportColumn column in data.Columns)
    {
        Cells[columnIndex][rowIndex].Value = column.ColumnName;
        Cells[columnIndex][rowIndex].Font.Bold = true; // put the header in bold
        for (int i = 0; i < column.ColumnData.Length; i++)
        {
            rowIndex++; // move down a cell to the next row
            Cells[columnIndex][rowIndex] = column.ColumnData.ElementAt(i);
        }
        rowIndex = startingRowIndex;
        columnIndex++; // move right a cell to the next column
    }

    Worksheet.Columns.AutoFit();
}

Below is the ReportData class (and associated ReportColumn class) which holds data retrieved from an external application to be inserted into a worksheet

public class ReportData
{
    public ReportData(IEnumerable<ReportColumn> columns)
    {
        Columns = columns ?? throw new ArgumentNullException("No valid columns available");
    }

    public IEnumerable<ReportColumn> Columns { get; set; }
}

public class ReportColumn
{
    public ReportColumn(string columnName, object[] columnData)
    {
        ColumnName = columnName;
        ColumnData = columnData;
    }

    public string ColumnName { get; set; }
    public object[] ColumnData { get; set; }
}

When trying to insert data into a worksheet which results in over 100k rows, there is a significant performance hit. Is there anyway I could improve the code to make it faster? Any other advice is welcomed too.

\$\endgroup\$
  • 2
    \$\begingroup\$ Try assigning Range.Value of the top-left target cell on the target sheet, to a 2D array that contains your values. That's 1 worksheet write operation instead of 100K x NumberOfColumns. Basically ReportData wants a ToArray() method. \$\endgroup\$ – Mathieu Guindon Jan 7 at 16:13
  • 1
    \$\begingroup\$ Also, how exactly are Cells and Worksheet defined? Seeing the rest of the code would be beneficial, too: why is the method working off whatever sheet happens to be active rather than from a specific worksheet? \$\endgroup\$ – Mathieu Guindon Jan 7 at 16:21
  • \$\begingroup\$ @MathieuGuindon Have included how Cells and Worksheets are defined. The method is working off the active sheet because there is no use case for the Add-In where we would want a non-active worksheet to be written to. \$\endgroup\$ – Danny Goodall Jan 8 at 9:00
  • 1
    \$\begingroup\$ @MathieuGuindon I have just tried your suggestion of only doing one worksheet write operation, and it is drastically improved. Thanks! \$\endgroup\$ – Danny Goodall Jan 8 at 11:00
2
\$\begingroup\$
public class ReportData
{
    public ReportData(IEnumerable<ReportColumn> columns)
    {
        Columns = columns ?? throw new ArgumentNullException("No valid columns available");
    }

    public IEnumerable<ReportColumn> Columns { get; set; }
}

The Columns property should be get-only.

public string ColumnName { get; set; } 
public object[] ColumnData { get; set; }

Same with these two: it makes no sense to set them in the constructor, even throw an exception if it's null, if you can legally construct the object and then merrily do this:

var data = new ReportData(Enumerable.Empty<ReportColumn>()) { Columns = null };

Note that IEnumerable<T> things should indeed never be null, but any IEnumerable<T> can still be empty without being null. As such, the guard clause is insufficient, since it appears to be there to protect against creating a new ReportData that wouldn't have any columns - Enumerable.Empty<T>() is a perfectly valid input that should be guarded against.

public class ReportData
{
    public reportData(IEnumerable<ReportColumn> columns)
    {
        var reportColumns = columns ?? throw new ArgumentNullException(...);
        if (!reportColumns.Any())
        {
            throw new ArgumentException(...);
        }

        Columns = reportColumns;
    }

    public IEnumerable<ReportColumns> Columns { get; }
}

Now there's no way a ReportData object can have 0 columns, be it during or after construction.

These two could be expression-bodied auto-properties:

private Worksheet Worksheet { get { return Globals.ThisAddIn.Application.ActiveSheet; } }
private Range Cells { get { return Globals.ThisAddIn.Application.ActiveSheet.Cells; } }

Like this:

private Worksheet Worksheet => Globals.ThisAddIn.Application.ActiveSheet;
private Range Cells => Globals.ThisAddIn.Application.ActiveSheet.Cells;

This is a serious problem:

// NOTE: Excel uses indexing that starts at 1
if (columnIndex == 0)
    columnIndex = 1;

if (rowIndex == 0)
    rowIndex = 1;

Consider consistently using { } braces to avoid such implicit scopes.

If the columnIndex or rowIndex supplied was 0, then the calling code is off-by-one and this silent one-upping is literally hiding that bug, and there's a very very very high chance that the next call is going to pass a 1 that you're not going to offset, which will produce a result, but very unlikely the expected result.

Consider:

  • Throwing an ArgumentOutOfRangeException given 0 for column or row index.
  • Adding XML docs to document that the index parameters are 1-based - OR document them as 0-based, and abstract away the fact that Excel's object model is 1-based by offsetting every provided value, not just 0. In any case, you need XML docs to say this.

My experience with VSTO is that if you don't properly release all the COM objects you ever access, then when your program exits you'll be left with a ghost EXCEL.EXE process in Task Manager, because .NET will be holding on to RCW objects wrapping COM/unmanaged objects that won't be garbage-collected like managed objects would be.

Hence, avoid double-dots like this:

Globals.ThisAddIn.Application.ActiveSheet

That's essentially leaking ThisAddIn, Application, and ActiveSheet objects.

Every single access to Cells is re-dereferencing the same ActiveSheet over and over and over, every time. That makes a lot of redundant member access.

var addin = Globals.ThisAddIn;
var application = addin.Application;
var activeSheet = application.ActiveSheet;

var allCells = activeSheet.Cells;
var cell = allCells[columnIndex][rowIndex];
cell.Value = column.ColumnName;

var cellFont = cell.Font;
cellFont.Bold = true;

...and then each of these objects need to be released... in the reverse order they were accessed:

Marshal.ReleaseCOMObject(cellFont);
Marshal.ReleaseCOMObject(cell);
Marshal.ReleaseCOMObject(allCells);
...

Miss one single COM object, and the EXCEL.EXE process won't be able to shut down correctly.

That makes it a pretty good reason to avoid writing to individual cells, ...regardless of the performance hit ;-)

| improve this answer | |
\$\endgroup\$

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.