Openfoodnetwork: [SPIKE] Performance – admin edit order cycles

[RachL]

Description
This issue covers a spike of the admin edit order cycles page.

Improving performance would address #3792.

Acceptances criteria
We need to have map the main painpoints and open issue for each of them. The scope of this issue does not require to find solutions for these.

Related PRs (with current status)

• #3880 - Code Review: Batch create and delete exchange variants
• #3825 - Can be Revisited: Missing table index
• #3818 - Can be Revisited: Missing table index
• #3802 - Merged: Remove useless query per AJAX request; Various OC-related optimizations targeting improvements in _loading_ time of edit OC page

[RachL]

@Matt-Yorkley @mkllnk @luisramos0 @sauloperez do you have things to add to describe this? Do we need a "spike" template?

[kristinalim]

I added "Related PRs" above and also linked to the issue #3792 for Germany.

[Matt-Yorkley]

I had a quick look for performance quick-wins, and addressed the easy ones in PR: #3883. Kristina also has a nice related PR #3880. I'll document the other things I found.

The order cycles edit page calls a few different endpoints that load various bits of data. Some of these endpoints go through a mad journey across the codebase, and create some nasty N+1 queries. We can't really have that if we want to load 10000 products. We can take a look at fixing those, but I think at some point we'll need a big rethink of the whole page.

Pain point: /admin/enterprises/for_order_cycle.json

The app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb calls these methods on every variant the enterprise owner has access to:

lib/open_food_network/variant_and_line_item_naming.rb:13:in `options_text'
lib/open_food_network/variant_and_line_item_naming.rb:42:in `unit_to_display'
lib/open_food_network/variant_and_line_item_naming.rb:30:in `full_name'
app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb:22:in `block in variants'
app/serializers/api/admin/for_order_cycle/supplied_product_serializer.rb:22:in `variants'
app/controllers/admin/enterprises_controller.rb:100:in `block (2 levels) in for_order_cycle'
app/controllers/admin/enterprises_controller.rb:98:in `for_order_cycle'

SELECT "spree_option_values".* FROM "spree_option_values" INNER JOIN "spree_option_types" ON "spree_option_types"."id" = "spree_option_values"."option_type_id" INNER JOIN "spree_option_values_variants" ON "spree_option_values"."id" = "spree_option_values_variants"."option_value_id" WHERE "spree_option_values_variants"."variant_id" = 203369 ORDER BY spree_option_types.position asc; 

It's fetching variant.fullname via the join tables: spree_option_values_variants and spree_option_types in variant_and_line_item_naming.rb. I tried doing some simple preloading for these values before the serialization takes place, but it didn't work...

Pain point: /admin/order_cycles/<order_cycle_id>.json (show)

This uses the app/serializers/api/admin/order_cycle_serializer.rb serializer, and has more N+1 queries

lib/open_food_network/order_cycle_permissions.rb:27:in `visible_enterprises'
app/serializers/api/admin/order_cycle_serializer.rb:37:in `editable_variants_for_incoming_exchanges'
app/controllers/spree/admin/base_controller_decorator.rb:81:in `render_as_json'
app/controllers/admin/order_cycles_controller.rb:24:in `block (2 levels) in show'
app/controllers/admin/order_cycles_controller.rb:21:in `show'

SELECT DISTINCT "enterprises"."id" FROM "enterprises" INNER JOIN "exchanges" ON "enterprises"."id" = "exchanges"."sender_id" WHERE "exchanges"."order_cycle_id" = 200066 AND "exchanges"."incoming" = 't'; 

There's some N+1 queries here that need fixing, in app/serializers/api/admin/order_cycle_serializer.rb, in the methods: #editable_variants_for_incoming_exchanges and #editable_variants_for_outgoing_exchanges.

Pain point: dealing with vast amounts of objects in Angular.

This part is basically a "client-side Angular objects memory bloat" issue.

• Kristina has a PR that should help with saving the OC.
• Luis suggested elsewhere that we can use Rack::Deflater to improve the speed when serving large mounts of json from our endpoints. We should do that. The optimal solution is slightly different for Rails 3 and Rails 4, so we should keep that in mind. More info here: https://thoughtbot.com/blog/content-compression-with-rack-deflater

We can't really do pagination on this page in the traditional sense, but there may be some things we can do to improve it. It would probably involve a big rethink and rewrite of the page though.

For example, we currently preload basically everything into Angular. We could switch to doing quick API calls for some of the UX parts, like for adding a product to the OC or selecting an enterprise, we could use a dropdown/search that gets populated from a query. We use a similar thing elsewhere in the site.

We could also split the incoming and outgoing exchange sections into two pages, so we don't deal with everything at once?

[mkllnk]

Pain point: dealing with vast amounts of objects in Angular.
Luis suggested elsewhere that we can use Rack::Deflater to improve the speed when serving large mounts of json from our endpoints.

I don't understand how that can help us here.

1. The problem is that Angular has a lot of data to process. That won't change by compressing the data for transit. It will be uncompressed before Angular sees it. Of course, quicker transfer time is always good.
2. Nginx is probably better suited to perform content compression. I don't know but maybe we are doing that already.

[Matt-Yorkley]

Yes, it won't solve the main problem but might improve the overall load time a bit.

[luisramos0]

I think one of the bottlenecks is that the json file is 0.5MB and that can be compressed to improve performance. In the test I did (I am not sure what thread it was), the problem was not angular handling the data, it was the server load time of the json - 5secs for 300+ objects.

[mkllnk]

Okay. So we have the generation of the JSON and the transfer. Regarding the transfer, I just just checked Australian production and it's still using HTTP/1.1. Then I checked Katuma and it's using HTTP/2. That's probably due to a newer nginx version. Or did you do some tweaks @sauloperez?

Anyway, I think I just read that https is always using gzip compression anyway. So my point is that we don't need to deal with compression (Rack Deflater) within the Rails application. But I might be wrong.

[luisramos0]

"I think I just read that https is always using gzip compression anyway"
I'd love to learn that but I'd say it's wrong. Can you share a link?
I think we need to enable gzip and that it will have a very good impact.
In my quick investigation I ended up reading about brotli

HTTP/2 is a different thing and yes we should enable it ❤️

[luisramos0]

I went to page speed insights to confirm. Compression is listed on the mobile evaluation:

(btw, our mobile shopfront page ranks 19/100 👀 )

[mkllnk]

Interesting. I thought that we had compression on:

https://github.com/openfoodfoundation/ofn-install/blob/91fde83e5df7464ce467dafea738d6c750f00ca0/inventory/group_vars/all.yml#L198

But according to the nginx documentation we are missing a line here:

gzip_proxied no-cache no-store private expired auth;

https://docs.nginx.com/nginx/admin-guide/web-server/compression/

[Matt-Yorkley]

In my quick investigation I ended up reading about brotli

Nice, we could probably add Brotli support quite easily.

HTTP/2 is a different thing and yes we should enable it heart

It's already enabled on all servers except Aus.

according to the nginx documentation we are missing a line...

I'll add it now.

[Matt-Yorkley]

I've opened two issues (#4006 and #4007) for the main two pain points identified in this spike, and we've upgraded our use of compression to alleviate the third pain point (in https://github.com/openfoodfoundation/ofn-install/issues/464 and https://github.com/openfoodfoundation/ofn-install/issues/465).

[mkllnk]

Nice work!