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

Read with batches

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

Shadowing

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?

Where does your data go?

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

Filter your data

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.

StackExchange Code Review Q#45276, answer score: 6