2

I'm having these models and following lines in a method where I am trying to improve performance of the code. 

class Location < ActiveRecord::Base belongs_to :company end class Company < ActiveRecord::Base has_many :locations end

In the method:

locations_company = [] ### found_locations = Location.within(distance, origin: from_result.split(',')).order("distance ASC") ### 0.002659s ### found_locations.each do |location| locations_company << location.company end ### 45.972285s ### companies = locations_company.uniq{|x| x.id} ### 0.033029s

The code has this functionality - first, grab all locations within a specified radius. Then, from each found row take the company and save it to the prepared array. This is the problematic part - the each loop takes 45 seconds to process.

And then from this newly created array remove duplicities.

I still wondering if there would be a better approach to solve this situation, but I am afraid I don't see it right now, so I'd like to ask you guys how I could speed up the .each looping with saving data to the array - is there a better method in ruby to grab some information from an object?

Thank you very much for your time, I am immersed in this problem the whole day, but still don't have a more effective solution.

Improve this question
2
  • If you look at found_locations you'll notice that it's likely a query proxy, and not a coalesced result set. The #each is almost certainly not your bottleneck; you should properly profile your code to find the bottlenecks. Commented Nov 20, 2014 at 19:54
  • This question appears to be off-topic because it is about refactoring and improving performance of existing code, and should be on Code Review. Commented Nov 20, 2014 at 22:25

3 Answers 3

Reset to default
7

The best way would be to not loop. Your end goal appears to be to find all the companies in the specified area.

found_locations = Location.within(distance, origin: from_result.split(',')).order("distance ASC") companies = Company.where(id: found_locations.pluck(:company_id).uniq)
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

A Company.distinct instead bleh.uniq may be helpful, if supported by the db.
Company.distinct is not needed. If you leave off the uniq it just passes a larger array into the WHERE id IN [] query. The database will only return one record per company even if the ids are included more than once in the array. I just personally don't like sending more information into the query than necessary and adding/removing uniq is not going to materially effect the performance.
Depending on if found_locations will actually be used or not in addition to the companies, you might take different variations on this. If you plan on using found_locations separately, then you can/should force it to an array with to_a and then change the logic in the next line to be Company.where(id: found_locations.map(&:id).uniq). If you don't plan on using found_locations separately, then what I put there would be best as then you won't even create the Location objects and instead just pull the ids that you need.
Another peachy-keen answer!
1

The problem is not in the each, but in that the query only begins executing when you start iterating over it. found_locations is no the result of the query, it is a query builder that will execute the query once it is needed (such as when you start iterating the results).

Improve this answer

Comments

1

I believe the thing that takes all the time is not the each, but rather the query to the db.

The first line, although it builds the query does not really run it.

I believe that if you write the code as follows:

locations_company = [] found_locations = Location.within(distance, origin: from_result.split(',')).order("distance ASC") ### this line will take most of the time found_locations = found_locations.to_a ### ### found_locations.each do |location| locations_company << location.company_id end ### ### companies = locations_company.uniq{|x| x.id} ###

You'll see that the each will take a lot less time. You should look into optimizing the query.

As @AlexPeachey has commented below, location.company will also involve a query for each location in the list, since it is a relation. You might want to eagerly load the company by adding:

found_locations = Location.includes(:company).within(distance, origin: from_result.split(',')).order("distance ASC")
Improve this answer

2 Comments

The query may well be slow, however the each will not be immediate with this approach because you are doing a query against the company table each time through the each loop. To avoid this, changing to Location.includes(:company) will load all of the companies required with just one additional query.
Thanks @AlexPeachey, I missed that part. Updated the answer

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.