Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Before looping through an Excel document using a library I found, and wanted to know how long it would take to loop through the whole thing.

There are 40k rows.

I looped through only the first 5k and it took just 2 minutes. So, I thought, that looping through 40k would take just 16 minutes total.

However, the program has been running for over an hour now, and it still hasn't completed.

What would be slowing this thing down? Granted, I didn't program it for high efficiency, but there must be something that is slowing this down... Why wasn't my initial estimate accurate?

Here is my method:

public static void Validate(ExcelConfiguration config)
{
    var ctx = new IntakeEntities();
    Stopwatch sw = new Stopwatch();
    sw.Start();
    HashSet<string> missingEntities = new HashSet<string>();

    Console.WriteLine("Validating...");

    using (ExcelPackage xlPackage = new ExcelPackage(config.file))
    {
        ExcelWorksheet worksheet = xlPackage.Workbook.Worksheets[config.WorkSheet];

        // loop through each row
        for (int iRow = config.DataRowStart; iRow <= config.DataRowFinish; iRow++)
        {
            // get home country from database
            string homeCountryName = worksheet.Cell(iRow, config.HomeCountryColumnIndex).Value.Trim();
            Guid? homeCountryId = null;

            try
            {
                homeCountryId = ctx.Countries.AsNoTracking().Where(x => x.Name == homeCountryName).First().Id;
                //Console.WriteLine(homeCountryId + " - " + homeCountryName);
            }
            catch (Exception ex)
            {
                //Console.WriteLine("No record found for " + homeCountryName);
                missingEntities.Add(homeCountryName);
            }

            // get host country from database
            string hostCountryName = worksheet.Cell(iRow, config.HostCountryColumnIndex).Value.Trim();
            Guid? hostCountryId = null;

            try
            {
                hostCountryId = ctx.Countries.AsNoTracking().Where(x => x.Name == hostCountryName).First().Id;
                //Console.WriteLine(hostCountryId + " - " + hostCountryName);
            }
            catch (Exception ex)
            {
                //Console.WriteLine("No record found for " + hostCountryName);
                missingEntities.Add(hostCountryName);
            }

            if (config.HomeLocationColumnIndex != null && config.HostLocationColumnIndex != null)
            {
                // get home country from database
                string homeLocationName = worksheet.Cell(iRow, (int)config.HomeLocationColumnIndex).Value.Trim();
                Guid? homeLocationId = null;

                try
                {
                    homeLocationId = ctx.Locations.AsNoTracking().Where(x => x.Name == homeLocationName).First().Id;
                    //Console.WriteLine(homeCountryId + " - " + homeCountryName);
                }
                catch (Exception ex)
                {
                    //Console.WriteLine("No record found for " + homeLocationName);
                    missingEntities.Add(homeLocationName);
                }

                // get host country from database
                string hostLocationName = worksheet.Cell(iRow, (int)config.HostLocationColumnIndex).Value.Trim();
                Guid? hostLocationId = null;

                try
                {
                    hostLocationId = ctx.Locations.AsNoTracking().Where(x => x.Name == hostLocationName).First().Id;
                    //Console.WriteLine(hostCountryId + " - " + hostCountryName);
                }
                catch (Exception ex)
                {
                    //Console.WriteLine("No record found for " + hostLocationName);
                    missingEntities.Add(hostLocationName);
                }
            }
        }
    }
    Console.WriteLine("Validation complete...");
    Console.WriteLine("Time elapsed: {0} minutes", sw.Elapsed.Minutes);

    Console.WriteLine("No records found for:");
    foreach (string entity in missingEntities)
    {
        Console.WriteLine(entity);
    }
}

Here is the accompanying object:

public class ExcelConfiguration
{
    public FileInfo file { get; set; }
    public int HomeCountryColumnIndex { get; set; }
    public int HostCountryColumnIndex { get; set; }
    public int? HomeLocationColumnIndex { get; set; }
    public int? HostLocationColumnIndex { get; set; }
    public int WorkSheet { get; set; }
    public int DataRowStart { get; set; }
    public int DataRowFinish { get; set; }
}
share|improve this question
add comment

4 Answers 4

  • Have performance issues? Use a profiler. (There is a built-in in certain versions of Visual Studio and a bunch of third-party tools, with evaluation times). Find out what is taking time and we can give you an advice for optimization after that.
  • As already mentioned, do not use exceptions for normal program flow. Use FirstOrDefault() method instead of First() so you don't get an exception for missing items.
  • If you want a guess - you have 4 database queries per row => 160k queries. Either adjust database indexes, or load whole tables into Dictionary<> if they are small enough (Countries table, for example, is probably small)
share|improve this answer
1  
+1 Good call, caching the smaller data sets locally, but .FirstOrDefault().Id will still potentially throw a NullReferenceException - the trick is to do .FirstOrDefault() (or .SingleOrDefault()), check if we have a result, and then if we do, get the .Id. –  Mat's Mug Apr 16 at 18:23
    
Yep, of course I meant FirstOrDefault()->Null check-> .Id –  Kefir Apr 16 at 18:25
    
+1, Very good advice here. The only thing is, I already tried loading the entire tables into a local cache both the method you described and using .Load() of Linq. I used the StopWatch() to compare and actually, both methods were slower than the one I am using now. Do you have a recommended implementation for this? –  user1477388 Apr 16 at 18:28
    
+1 for use a profiler. red-gate.com/products/dotnet-development/… is the best by leaps and bounds, in my opinion. –  Reinderien Apr 16 at 18:28
1  
Another question: After profiling using the VS2012 Analyzer, it tells me that most of the problem is the accessing of the Excel Cells. But, how can I avoid that? –  user1477388 Apr 16 at 20:25
add comment
public static void Validate(ExcelConfiguration config)

I wouldn't make this a static method, but without knowing more about your project it's just a nitpick. The name is highly misleading though. Without looking at the code, one would expect that the method validates the passed ExcelConfiguration - I don't know, maybe check that DataRowStart is within a specific range (like, a positive number), and that the File (capital F) exists... wait, you've got a FileInfo there, that's rather surprising. Also WorkSheet should be called WorksheetIndex... but I digress.

var ctx = new IntakeEntities();

I like that you're using var. However if this IntakeEntities is what I think it is, it's derived from DbContext and implements IDisposable, and should be wrapped in a using block.

Stopwatch sw = new Stopwatch();

Why not use var again? Be consistent! That said, you're doing the right thing - StopWatch is how you should time execution... but that stop watch should be in the method's calling code, starting just before the method call and stopping right after it returns. Writing the results to the console doesn't belong in that method either.


Are you actually catching many exceptions? Are exceptions exceptional or just a "neat" way of adding items to missingEntities?

I think you're misusing exceptions, because if you didn't systematically tried to get the Id property of an object that's potentially null, you could do a null check instead, and to everything without catching a NullReferenceException - use SingleOrDefault instead of Where and then First, it makes it clearer that you're expecting zero or one values:

var homeCountry = ctx.Countries.AsNoTracking().SingleOrDefault(x => x.Name == homeCountryName);
homeCountryId = (homeCountry == null ? 0 : homeCountry.Id);

if (homeCountryId == 0)
{
    missingEntities.Add(homeCountryName);
}

Throwing and catching exceptions without a real reason, in a loop, 40K times, can slow things down if half the iterations throw 2-3 exceptions. I'd try to eliminate the throwing and catching first, see what I get.

Then I'd try to reduce the number of Excel worksheet reads, if that's at all possible.

share|improve this answer
    
Thanks so much. I have confidence in your answer. I will try it soon. –  user1477388 Apr 16 at 18:28
    
I tend to disagree with the use of var for known types: from c#3.0 language reference... "Overuse of var can make source code less readable for others. It is recommended to use var only when it is necessary, that is, when the variable will be used to store an anonymous type or a collection of anonymous types." –  Nick Williams Apr 16 at 21:37
1  
Also while using FirstOrDefault(x => x.Name == "Foo"); might look neater, using Where(x => x.Name == "Foo").FirstOrDefault(); is actually quicker. See here. A micro optimisation but on a 40k loop with 4 calls might make a difference. –  Nick Williams Apr 16 at 21:41
1  
@NickWilliams I tend to disagree with what's considered "overuse" of var. I don't see how cutting redundant clutter reduces readability, and using var or not boils down to personal preference - in my answer I only recommended consistency in its usage. –  Mat's Mug Apr 16 at 22:38
1  
@Mat'sMug I would always follow the language reference on these matters if trying to suggest best practise. Most (not all) code I have seen where var is used for every variable ends up with longer, more Hungarian style variable names. The main thing is to be consistent of course. –  Nick Williams Apr 17 at 8:09
add comment
up vote 3 down vote accepted

With appreciation for the answers and sound advice provided here, I have concluded that the real source of the issue was not my improper use of try/catch nor FirstOrDefault() or any of that, but rather, it was the Excel package.

To fix this, the only solution would be to convert the Excel into a CSV which would allow for faster access of data. Instead of that, I just chose to modify my DataRowStart and DataRowFinish by shrinking the window so it wouldn't get bogged down.

Update:

After converting my document to CSV and using FileHelpers and changing the runtime configuration of my DbContext like so:

yourContext.Configuration.AutoDetectChangesEnabled = false;
yourContext.Configuration.ValidateOnSaveEnabled = false;

... I was able to validate and insert all 40k records in under 15 minutes; whereas, before it would've taken hours (if it finished at all). I think most of the benefit actually came from the DbContext change. That alone was a major improvement.

share|improve this answer
    
did you try to fix the try catch usage? –  Malachi Apr 17 at 15:06
1  
Yes, it helped some, but according to the built in Profiler in VS 2012, most of the time was taken up by the reads to the Excel document, not by database calls or try/catch, etc. –  user1477388 Apr 17 at 15:08
2  
That doesn't surprise me; as I explained in this answer the real cost of throwing+catching exceptions is often over-estimated. I thought 40K iterations would actually make a difference, but I under-estimated the cost of COM interop: "Then I'd try to reduce the number of Excel worksheet reads, if that's at all possible." - that should have been my first advice ;) –  Mat's Mug Apr 17 at 15:30
1  
glad you got it going. –  Malachi Apr 17 at 18:35
    
Thanks. I posted an update since I thought it was really cool. By making those changes, I was able to complete the entire operation in under 15 mins. –  user1477388 Apr 17 at 19:02
show 1 more comment

The one thing that people are touching on but not saying outright is the fact that Exception handling takes a lot of processing power away from what you want going on, just the operation of catching the exception itself is what is slowing you down.

you are using Catch statements inappropriately.

if you know the exception then you need to code so that it won't happen.

a try catch statement is more for the unexpected errors.

like @Mat's Mug was saying, check to see if it exists, if it doesn't add it to the missing entities collection, don't let it throw an error.


as far as your estimate being wrong.

I looped through only the first 5k and it took just 2 minutes. So, I thought, that looping through 40k would take just 16 minutes total.

the issue with this is that for the first 5k records there is (theoretically) nothing being held in memory or processor cache or heap, so when the next 5k records go through there is stuff being processed already these 5k records have to wait in line, stack another 5k on top of that, now the processor is getting warm which slows it down a little bit, it's trying to multi thread information is getting backed up. it's like having 40 tabs open on a browser, I am done with the first 35 but have things running super slow on the remaining 5 it's slow because there is stuff in the way sitting in the receiving and sending area.

okay so I BS'd my way through that, but you get the point right?

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.