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

Refactor HistoricalTrendService for performance optimization #4131

Merged
merged 2 commits into from
Feb 25, 2024

Conversation

phonghpham
Copy link
Contributor

@phonghpham phonghpham commented Feb 21, 2024

Resolves #4116

Description

Issue #4116 highlighted prformance concerns with HistoricalDataCacheJob. Upon investigation, this job calls out to HistoricalTrendService where n+1 query was identified when items were queried and then for each item a query made to check if there are eligible line_items for item. New implementation replaces this check by preloading filtered line_items.

Additionally, moved aggregation of line_items into ruby instead of via db queries to reduce db load.

Benchmark measure of historical_trend_service_spec.rb call to HistoricalTrendService.series results before refactoring:

  #series performance
  0.087621   0.008420   0.096041 (  0.147053)
  0.014439   0.001048   0.015487 (  0.020131)
  0.014943   0.001097   0.016040 (  0.020658)
  0.015538   0.001313   0.016851 (  0.021404)
  0.013140   0.001055   0.014195 (  0.018298)
  0.012821   0.001000   0.013821 (  0.017912)
  0.012873   0.001067   0.013940 (  0.019809)
  0.012985   0.001229   0.014214 (  0.019213)
  0.014079   0.000990   0.015069 (  0.019707)
  0.012705   0.001032   0.013737 (  0.018755)

and after:

  #series performance
  0.067465   0.006804   0.074269 (  0.117842)
  0.000854   0.000076   0.000930 (  0.001406)
  0.000619   0.000026   0.000645 (  0.001147)
  0.000517   0.000025   0.000542 (  0.001021)
  0.000548   0.000047   0.000595 (  0.001053)
  0.000502   0.000018   0.000520 (  0.000970)
  0.000494   0.000021   0.000515 (  0.001167)
  0.000535   0.000034   0.000569 (  0.001194)
  0.000488   0.000025   0.000513 (  0.001114)
  0.000480   0.000020   0.000500 (  0.001255)

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • existing tests not broken by refactor

@dorner
Copy link
Collaborator

dorner commented Feb 22, 2024

Looks good! Lint is failing though 😄

This commit addresses issue regarding performance of
HistoricalDataCacheJob by optimizing HistoricalTrendService#series
method while still maintaining existing functionality and data
structure returned by HistoricalDataCacheJob#series.

- preload `line_items` to avoid N+1 query
- use ruby for aggregating line_item qty to reduce db queries
- init month_offset and default_dates once outside of loop
@phonghpham phonghpham force-pushed the 4116-historical-cache-job-issue branch from 0d30007 to f97732d Compare February 22, 2024 18:00
@awwaiid awwaiid merged commit 1734e0d into rubyforgood:main Feb 25, 2024
19 checks passed
Copy link
Contributor

github-actions bot commented Mar 3, 2024

@phonghpham: Your PR Refactor HistoricalTrendService for performance optimization is part of today's Human Essentials production release: 2024.03.03.
Thank you very much for your contribution!

@cielf
Copy link
Collaborator

cielf commented Mar 10, 2024

Hey @phonghpham Just thought you'd like to know that in the week since your fix was released, we have had no time out errors on the historical trends. Thanks again!

@phonghpham phonghpham deleted the 4116-historical-cache-job-issue branch March 14, 2024 18:40
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.

[perf] Historical cache job is taking over 5 minutes. This seems excessive.
4 participants