# Speed up long running Ruby code

I'm running code which works fast locally, but when it's on Heroku it's slow. Heroku says this is because there is less memory and CPU power on my hosting than locally. I'm trying to rewrite my code so it's more efficient, but I'm not sure how.

This is what they said:

...[my] method (especially, a.each part) is taking time. I think a itself is pretty long, and you're transpose it with subs.keys, and do some process for each of them. Even though s.gsub!(*pair) is not "long running" code, if you run this million times (actually runs 1,161,216 times for the word "startup"), it'll be "slow" request.

Here is my code:

def substitute(word)
subs = Hash.new
Section.all.to_a.each do |s|
alternates_array = Array.new
s.substitutions.each do |alt|
alternates_array << alt.alternate
end
alternates_array << s.part
new_hash = Hash.new
subs[s.part] = alternates_array
end
a = subs.values
a = a.first.product(*a.drop(1))
alternates = Array.new
a.each do |a|
new_string = [subs.keys, a].transpose.each_with_object(word.dup){|pair, s| s.gsub!(*pair)}
end
return alternates
end


Section Model

class Section
include Mongoid::Document
field :part, type: String

embeds_many :substitutions

accepts_nested_attributes_for :substitutions
end


Substitution Model

class Substitution
include Mongoid::Document
field :alternate, type: String

embedded_in :section
end

• Section model is added. – user2270029 Mar 25 '14 at 3:18
• Could you add some explanation what your code is supposed to do? This will make it easier for people to review it. – ChrisWue Mar 25 '14 at 6:54

Looping over Section.all.to_a is very memory greedy (it reads everything to memory before starting to process it). You will be far better off using batches:

Section.find_each do |section|
# ...
end


Naming
Using member names like s, a, etc. is not very readable, and makes understanding what your code does very difficult. For that reason, it is very hard for us reviewers to give you an intelligent advice on how your code can be improved - we just don't understand what it does...

a.each do |a| - to add insult to injury, you shadow the a member when you loop over it. This makes your code totally unreadable. I still don't understand what that loop is all about.

Loop bloat
The line a = a.first.product(*a.drop(1)) is very suspicious, since it bloats the amount of data your are looping on by several scales. Are you sure that is what you want to do?

The new_string, which holds the result for each iteration is not seemed to be saved anywhere...

At the bottom line, you are substituting words in your input. Not all sections exist in all inputs, but you search for them over and over again. You might save a lot of calculations if you filter irrelevant sections in your code, before looping on them:

Section.find_each do |section|
next unless word[section.part]
alternate_array #...
end


Doing stuff over and over again
I'm pretty sure, though I might be wrong, that at the end s.gsub!(*pair) ends up running the same "pairs" more than once on the same string (or its duplicates). It will be better strategy to cache results, or maybe pre-process your data (build a template, and only set the variables, or something).

Edit
You could use a variant of gsub! which receives a hash as the replacement:

If the second argument is a Hash, and the matched text is one of its keys, the corresponding value is the replacement string.

So you could write:

replace_regexp = Regexp.union(subs.keys)

replaced_words = a.map do |alts|
word.gsub(replace_regexp, Hash[[subs.keys, alts].transpose])
end


This does a single gsub per combination (instead of once per key), and further reduces the run complexity of your code.