1
\$\begingroup\$

I have the following code and it's taking quite some time to run. I'm getting an Out of Memory Exception due to the high volume of answers there are. Is there any way I could speed it up?

var controlStrings = answerControlStrings.Where(a => a.ControlN.Substring(a.ControlN.IndexOf('_') + 1, a.ControlN.Length - a.ControlN.IndexOf('_') - 1).Equals(newAnswer.QVar, StringComparison.InvariantCultureIgnoreCase));
if (controlStrings.Count() > 0)
{
    newAnswer.CatiControlStrings.Add("en", controlStrings.Where(c => c.CultureN.Contains("cati") && c.CultureN.Contains("en")).ToList());
    newAnswer.WebControlStrings.Add("en", controlStrings.Where(c => c.CultureN.Contains("web") && c.CultureN.Contains("en")).ToList());
    newAnswer.CatiControlStrings.Add("es", controlStrings.Where(c => c.CultureN.Contains("cati") && c.CultureN.Contains("es")).ToList());
    newAnswer.WebControlStrings.Add("es", controlStrings.Where(c => c.CultureN.Contains("web") && c.CultureN.Contains("es")).ToList());
}
\$\endgroup\$
  • \$\begingroup\$ Is this called in some kind of loop? How is this data used? It would be helpful if you gave us more context here. How many answers are there? Please give us more information! \$\endgroup\$ – Simon Forsberg Feb 5 '14 at 18:29
  • \$\begingroup\$ If you posted more code, including what type answerControlStrings is, including how and/or how often this is called, including which statement is executing if/when an exception is thrown, and including what "high volume of answers there are" means more exactly, then the answer[s] might be more helpful. \$\endgroup\$ – ChrisW Feb 5 '14 at 21:46
11
\$\begingroup\$

You're iterating through controlStrings 4 times (because you have 4 Where clauses).

It might be better to rewrite this as a for loop:

foreach (var c in controlStrings)
{
    var cultureN = c.CultureN;

    if (cultureN.Contains("cati") && cultureN.Contains("en"))
        ... add c to CatiControlStrings;

    if (cultureN.Contains("web") && cultureN.Contains("en"))
        ... add c to WebControlStrings;

    ... and the same for Spanish.
}

You can also call Contains fewer times:

foreach (var c in controlStrings)
{
    var cultureN = c.CultureN;

    bool cati = cultureN.Contains("cati");
    bool web = cultureN.Contains("web");
    if (!cati && !web)
        continue;
    bool en = cultureN.Contains("en");
    bool es = cultureN.Contains("es");

    if (cati && en)
        ... add c to CatiControlStrings;

    if (web && en)
        ... add c to WebControlStrings;

    ... and the same for Spanish.
}

Also your Count() statement may be expensive and unnecessary.

You're calculating IndexOf twice for every string.

And you're creating a new Substring for every string you test. Instead of creating a substring, use one of the overloads of String.Compare which lets you compare substrings.

Alternatively something like this might be better (because it doesn't use Substring and is simpler than using IndexOf with Compare:

string compareWith = "_" + newAnswer.QVar;
var controlStrings = answerControlStrings.Where(a => a.ControlN.EndsWith(compareWith,
    StringComparison.InvariantCultureIgnoreCase));

Garbage, garbage, everywhere says that using a Where will generate a lot of temporary objects. I don't understand why you're getting "out of memory" sometimes, but rewriting even your first statement to use less Linq might help (or at least make the cause your "out of memory" exception clearer).

foreach (var a in controlStrings)
{
    if (a.ControlN.EndsWith(compareWith, StringComparison.InvariantCultureIgnoreCase))
    {
        // the add-to-dictionary code above
    }
}

Reading C# Does Lambda => generate garbage? (including comments to the accepted answer), Linq generates temporary garbage.

I have a suspicion, after reading Garbage Collection and Performance, that maybe the framework doesn't garbage-collect before throwing 'out of memory'; my guess is that:

  • Garbage collection runs when it feels like it
  • Garbage collection runs on a separate thread, without pausing/blocking your threads
  • Unlike Java, C# will/may not stop to garbage collect immediately before/instead of throwing 'out of memory exception'
  • Therefore if your thread generates too much garbage too fast (i.e. faster than the garbage is collected), it may throw.

An alternative theory is that garbage remains uncollectable inside the lambda instances until your var controlStrings goes out of scope.

Simplifying your code to use less Linq and no Substring instances should produce less garbage, which might help to avoid throwing the exception.

\$\endgroup\$
  • \$\begingroup\$ If you nest the if's with 'cultureN.Contains("en")' at the top you cut out one of the contains calls. \$\endgroup\$ – apieceoffruit Feb 6 '14 at 11:49
  • \$\begingroup\$ Thank you: I edited to remove several of the Contains calls. \$\endgroup\$ – ChrisW Feb 6 '14 at 12:03

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.