Improve effiency of OrderCycle.earliest_closing_times

[johansenja]

What? Why?

When visiting https://openfoodnetwork.org.uk/shops, the page load is very slow. Given that this page is linked to from the
CTA above the fold on https://openfoodnetwork.org.uk/, it seems ideal that should load quickly.

From my testing so far, it is significantly quicker to calculate the open shops by using pluck instead of a select and map - pluck uses less computation and memory, because it pulls the raw values rather than active record objects.

What should we test?

I did a bit of benchmarking myself, but feel free to run these as well to compare:

$ rails runner benchmark.rb
Running via Spring preloader in process 93743
              user     system      total        real
select map 18.210275   0.490729  18.701004 ( 21.091514)
pluck     5.991475   0.128221   6.119696 (  8.205571)
Number of OrderCycles: 80010

where benchmark.rb is:

require "benchmark"

OrderCycle.instance_eval do
  def self.earliest_closing_times_select_map
    Hash[
      Exchange.
        outgoing.
        joins(:order_cycle).
        merge(OrderCycle.active).
        group("exchanges.receiver_id").
        select("exchanges.receiver_id AS receiver_id,
                MIN(order_cycles.orders_close_at) AS earliest_close_at").
        map { |ex| [ex.receiver_id, ex.earliest_close_at.to_time] }
    ]
  end

  def self.earliest_closing_times_pluck
    Hash[
      Exchange.
        outgoing.
        joins(:order_cycle).
        merge(OrderCycle.active).
        group("exchanges.receiver_id").
        pluck(Arel.sql("exchanges.receiver_id AS receiver_id"),
              Arel.sql("MIN(order_cycles.orders_close_at) AS earliest_close_at"))
    ]
  end
end

N = 20

Benchmark.benchmark(Benchmark::CAPTION, 7, Benchmark::FORMAT) do |x|
  ActiveRecord::Base.uncached do
    x.report("select map") do
      N.times { OrderCycle.earliest_closing_times_select_map }
    end

    x.report("pluck") do
      N.times { OrderCycle.earliest_closing_times_pluck }
    end
  end
end

puts "Number of OrderCycles: #{OrderCycle.count}"

I'm not sure if ~80K order cycles would be a representative number for a production instance, but it seems like there is enough of a difference here for this to be useful?

Release notes

Changelog Category (reviewers may add a label for the release notes):

• Technical changes only

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[johansenja]

It looks like the failing spec is currently because of a rate limit, but hopefully if this is re-run it should pass 🤞🏻

[rioug]

Awesome ! thank you for looking into this 🙏

[mkllnk]

Great contribution, thank you! ✨

The unit test is good, too. The mocked data has the disadvantage of not simulating the data types returned from the database, e.g. testing to_time but it's better than before.

[mkllnk]

For testing:

• Visit /shops.
• Ensure that all data is still displayed as before.
• Maybe observe if it loads quicker with lots of data.

[drummer83]

Hi @johansenja,
Thank you for this exciting pull request!

I have tested it on our UK staging server and couldn't spot any difference, which is good in this case, I think. All data is there, the page is loading correctly. I can't really comment on the performance of the page.

Here are some screenshots (identical before and after):

Since no issues can be observed I trust in you and our reviews and I will go ahead and merge. 🚀
Thanks again! 🥳 🥂