Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

4210 Fetch and scope variants for shop in ProductsRenderer only once #4224

Conversation

kristinalim
Copy link
Member

@kristinalim kristinalim commented Sep 5, 2019

What? Why?

This caches in instance variable ProductsRenderer#all_variants_for_shop so that the variants do not have to be fetched and scoped twice for the two times that this method is called.

This is a small code change that could considerably reduce the RAM and processing done for RefreshProductsCacheJob. It takes some time for Ruby garbage collection to happen, so for a shop that has thousands of variants in their order cycle, this could mean number_of_variants + number_of_variant_overrides fewer objects in memory, in addition to the I think only slightly reduced number of SQL queries.

(Maybe garbage collection is something we could look into too, if we're not doing this already.)

What should we test?

For below:

  • Supplier stock settings - What you can set in Bulk Edit Products page
  • Variant override stock settings - What you can set in Inventory page

Do the following with an open OC:

  1. Create a new variant with unlimited stock (supplier).
  2. Add the variant to the OC. The variant should be available in the shop.
  3. Test the following:
    • 0 limited stock (supplier) - not available in shop
    • Positive limited stock (supplier) - available in the shop
  4. In the Inventory page, add a variant override for the variant.
  5. Test the following:
    • 0 limited stock (variant override) - not available in shop
    • Positive limited stock (variant override) - available in shop
    • 0 limited stock (variant override) again - wait for variant to disappear again
    • Unlimited stock (variant override) - available in shop

Release notes

  • Fetch and scope variants for shop in ProductsRenderer used in RefreshProductsCacheJob only once.

Changelog Category: Changed

@kristinalim kristinalim self-assigned this Sep 5, 2019
@kristinalim kristinalim force-pushed the feature/4210-fetch_and_scope_variants_once_in_products_renderer branch from 85d6a36 to 0138d07 Compare September 5, 2019 11:06
@sauloperez
Copy link
Contributor

sauloperez commented Sep 5, 2019

It takes some time for Ruby garbage collection to happen,

(Maybe garbage collection is something we could look into too, if we're not doing this already.)

Very good point. It's worth enabling Datadog's Ruby GC monitoring. According to https://docs.datadoghq.com/tracing/setup/ruby/ it will fetch Garbage collection statistics which will allow us to validate this claim.

@kristinalim kristinalim changed the title Fetch and scope variants for shop in ProductsRenderer only once 4210 Fetch and scope variants for shop in ProductsRenderer only once Sep 5, 2019
# because we need to look up the stock as overridden by
# VariantOverrides, and the scope method is not affected by them.
scoper = OpenFoodNetwork::ScopeVariantToHub.new(@distributor)
@all_variants_for_shop = Spree::Variant.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@all_variants_for_shop = is redundant here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I missed that. Yeah, that shouldn't be there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! Sorry, that was from an earlier change. Fixed already.

@sauloperez
Copy link
Contributor

sauloperez commented Sep 5, 2019

Unfortunately, until we don't get rid of VariantOverrides we have a blocker to properly improve the performance of many endpoints and jobs.

@kristinalim kristinalim force-pushed the feature/4210-fetch_and_scope_variants_once_in_products_renderer branch from 0138d07 to 590ce67 Compare September 5, 2019 14:43
@sauloperez sauloperez merged commit ba04208 into openfoodfoundation:master Sep 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants