CODEWITHSUNDEEP
ruby-on-railsrubyarraysperformanceeach
user984621
user984621

Reputation: 48453

Ruby - how to speed up looping through an ".each" array?

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.

Upvotes: 2

Views: 2871

Answers (3)

Uri Agassi
Uri Agassi

Reputation: 37409

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")

Upvotes: 1

Jakub Arnold
Jakub Arnold

Reputation: 87210

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).

Upvotes: 1

Alex Peachey
Alex Peachey

Reputation: 4686

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)

Upvotes: 7

Related Questions