4
\$\begingroup\$

I have an object that I am populating via mapping and lookup with Reflection:

this.SearchResults = (from a in response.postings
                          select new SearchResponseModel
                          {
                              Id = a.id,
                              TimeStampDate = a.timestampDate,
                              Body = a.body,
                              Title = a.heading,
                              Status = a.status,
                              State = a.state,
                              Language = a.language,
                              Currency = a.currency,
                              CategoryGroup = a.category_group,
                              Source = a.source,
                              ExternalId = a.external_id,
                              ExternalUrl = a.external_url,
                              Price = a.price,
                              Location = PopulateLocation(a.location)
                          }
                          ).ToList();

And the method that does the mapping:

 private static List<LocationLookupModel> PopulateLocation(Location location)
{
    List<LocationLookupModel> allLocations = new List<LocationLookupModel>();
    if (HttpContext.Current.Session["LocationModel"] == null)
    {

        HttpContext.Current.Session["LocationModel"] = allLocations = new LocationModel().LocationList;
    }
    else
    {
        allLocations = (List<LocationLookupModel>)HttpContext.Current.Session["LocationModel"];
    }
    List<LocationLookupModel> modelList = new List<LocationLookupModel>();

    foreach (PropertyInfo propertyInfo in location.GetType().GetProperties())
    {
        var value = propertyInfo.GetValue(location);
        if (value != null)
        {
            LocationLookupModel model = (from a in allLocations
                                         where a.Code == propertyInfo.GetValue(location).ToString()
                                         select a).FirstOrDefault();
            if (model != null)
            {
                modelList.Add(model);
            }
        }
    }

    return modelList;
}

The issue that I run into is that an allLocations object has about 70k records (it represents a list of location lookup values for countries, states, zipcodes, etc), and populating about 100 instances of SearchResponseModel takes about 20 seconds. This is far too long for a UI call, and I have not been able to find a way to make it faster. I understand I am basically doing 3 nested loops (calling helper method for each population, looping over reflected properties, and finally the LINQ call over 70k records) so there are some time efficiency issues, but I am a bit lost on what tricks I can use to make this process more efficient.

Here is a screenshot of the location object:

Location Object

Here is a screenshot of one of the allLocation objects in the list:

enter image description here

\$\endgroup\$
  • \$\begingroup\$ Could you describe in words what you're doing? It seems to me like you're doing way too much for what it actually is. Why are you looking through each property to find one with a valid code? Why don't you know beforehand which fields contain a code? \$\endgroup\$ – Jeroen Vannevel May 11 '15 at 14:54
  • \$\begingroup\$ @JeroenVannevel I have added some more detail, let me know if you need anything else. \$\endgroup\$ – Isaac Levin May 11 '15 at 15:25
2
\$\begingroup\$

Well to start with reflection is incredibly slow. And second I'm not even sure why you're using reflection to begin with since you have the strongly typed Location. Don't use reflection. I don't think you need to compare every single property. Pick only the ones that could possibly match.

If you insist on using reflection here's a faster impl:

private static readonly locationProperties = typeof(Location).GetProperties();

foreach (PropertyInfo propertyInfo in locationProperties)
{
    var value = propertyInfo.GetValue(location);
    if (value != null)
    {
        LocationLookupModel model = allLocations.FirstOrDefault(a.Code == value.ToString());
        if (model != null)
        {
            modelList.Add(model);
        }
    }
}

Without reflection:

private static List<LocationLookupModel> PopulateLocation(Location location)
{
    List<LocationLookupModel> allLocations = new List<LocationLookupModel>();
    if (HttpContext.Current.Session["LocationModel"] == null)
        HttpContext.Current.Session["LocationModel"] = allLocations = new LocationModel().LocationList;
    else
        allLocations = List<LocationLookupModel>)HttpContext.Current.Session["LocationModel"];
    return allLocations.Select(x => x.Code == location.city || x.Code == location.country || x.Code == location.county || x.Code == location.locality || x.Code == location.metro || x.Code == location.region || x.Code == location.state || x.Code == location.zipcode).ToList();
}
\$\endgroup\$
  • \$\begingroup\$ That snippet drops the speed down to 4 seconds, which is getting more manageable. Can you provide an implementation where I don't need Reflection? \$\endgroup\$ – Isaac Levin May 11 '15 at 22:08
  • \$\begingroup\$ @IsaacLevin That goes to show you how much reflection costs. We got a 400% speed increase removing a little bit of it. I'll edit with another solution shortly. \$\endgroup\$ – Zer0 May 11 '15 at 22:18
1
\$\begingroup\$

@Zer0's answer will definitely give you the largest performance boost. Another way to help with performance is to run them in parallel.

var responseCollection = new ConcurrentBag();

Parallel.Foreach(response.Postings, item => 
{
    responseCollection.Add(new SearchResponseModel { //blah });
}

this.SearchResults = responseCollection.ToList();

This will let you process all 100 instance (or 70K? Kind of confused on that part) in parallel. In this case, when combined with @Zer0's answer, I think that will help get you under the 4 seconds.

You would use a ConcurrentBag in this scenario, and then convert it to a collection when you are finished processing it.

If you are concerned with it hanging on your UI, you can run it asynchronously. I can't see your method signature that is actually processing the items, so there are a couple ways to handle it.

public async Task Process()
{
    var responseCollection = new ConcurrentBag();
    await Task.Run(() =>
    {
        Parallel.Foreach(response.Postings, item => 
        {
            responseCollection.Add(new SearchResponseModel { //blah });
        }
    });

    this.SearchResults = responseCollection.ToList();
}

If you are running in a void method, you could use a Task and continuation.

public void Process()
{
    var responseCollection = new ConcurrentBag();
    Task.Run(() =>
    {
        Parallel.Foreach(response.Postings, item => 
        {
            responseCollection.Add(new SearchResponseModel { //blah });
        }
    })
    .ContinueWith(task => { this.SearchResults = responseCollection.ToList(); });
}

You can also marshall the results back to the UI thread if the SearchResults is bound in anyway to the UI. This will at least make the UI responsive while the app processes for a couple seconds.

\$\endgroup\$
  • \$\begingroup\$ Using ConcurrentBag would be great, but how is that going to work with the call to get the Session? If I take your code, the Current HttpContext is null. Since that is needed to populate the location data, should I be storing it somewhere other than the Session? \$\endgroup\$ – Isaac Levin May 12 '15 at 15:01
  • \$\begingroup\$ You could also use PLINQ instead of Parallel.ForEach : this.SearchResults = response.Postings.AsParallel().Select(item => new SearchResponseModel{ }).ToList(); \$\endgroup\$ – Dan Lyons May 12 '15 at 18:07
  • \$\begingroup\$ I would avoid this usage of ConcurrentBag due to the performance hit. It is optimized for the scenario where a given thread is both a producer and consumer because it uses thread local storage. Whereas this design has distinct producer and consumer threads. \$\endgroup\$ – Zer0 May 12 '15 at 18:41
0
\$\begingroup\$

Zer0's improvements can be further reduced: there's no need to create a modelList, then fill it using AddRange and then return it. Simply do all that in one go:

return allLocations.Select(x => x.Code == location.city 
    || x.Code == location.country 
    || x.Code == location.county 
    || x.Code == location.locality 
    || x.Code == location.metro 
    || x.Code == location.region 
    || x.Code == location.state 
    || x.Code == location.zipcode).ToList();

HttpContext.Current.Session["LocationModel"] is used three times, so why not assign it to a variable?

var locationModel = HttpContext.Current.Session["LocationModel"];

if(locationModel == null)
{
   HttpContext.Current.Session["LocationModel"] = allLocations = new LocationModel().LocationList;
}
else
{
   allLocations = List<LocationLookupModel>)locationModel;
}

But that logic still feels convoluted. I'd extract it to a separate method:

private const string _sessionName = "LocationModel";

private List<LocationLookupModel> GetOrCreateLocationModelSession()
{
    var locationLookupModels = (List<LocationLookupModel>)(
        HttpContext.Current.Session[_sessionName]
            ?? new LocationModel().LocationList);

    HttpContext.Current.Session[_sessionName] = locationLookupModels;

    return locationLookupModels;
}

Also: wouldn't it make sense to replace the List<LocationLookupModel> with a Dictionary<Location, List<LocationLookupModel>>? That is: assuming the Location parameter isn't always unique.

I wonder about the wisdom of storing 70k records in a session variable, wouldn't it make more sense to look at improving the performance of new LocationModel().LocationList -- e.g. via a cache,... -- and simply storing the result of allLocations.Select(x => /* query */).ToList(); in the session if you expect repeated calls for the same Location?


I'm a bit worried by location.country, location.locality etc.: I'm assuming these are public properties, which means country and locality etc. should be PascalCase.

\$\endgroup\$
  • \$\begingroup\$ Good points. I edited for that optimization. \$\endgroup\$ – Zer0 May 12 '15 at 18:36

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.