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

Revert "Use the tag_list method, rather than the cached_tag_list attribute" #15681

Closed

Conversation

djuber
Copy link
Contributor

@djuber djuber commented Dec 3, 2021

Reverts #15638

Non-critical - but the performance did decrease more than my limited local testing suggested it would.

https://ui.honeycomb.io/thepracticaldev/datasets/rails/result/5v8bxRJqoBy

Screenshot from 2021-12-03 16-55-09

Comparison with the day before versus after 15638 deployed:
Screenshot from 2021-12-03 17-00-07

This job (Follows::UpdatePointsWorker) is now taking between 0.5-2.0 seconds - we run it about 7000 times a day (averaging to about every 10 seconds), so it's not a huge problem, but it's a bigger regression in performance than we should accept uncritically

@pr-triage pr-triage bot added the PR: draft bot applied label for PR's that are a work in progress label Dec 3, 2021
@citizen428 citizen428 requested review from a team and jgaskins and removed request for a team December 6, 2021 04:10
@citizen428
Copy link
Contributor

This is what I was afraid of :-( Can we easily revert this or will this bring back a problem that the previous PR will revert? Also, is this performance regression something critical or can we live with it?

@djuber
Copy link
Contributor Author

djuber commented Dec 6, 2021

Ah - this is

  1. something we can live with (the performance regression is acceptable for how often the job runs) - that's my personal gut feeling and can be invalidated by any counter-opinion.
  2. something we can safely revert without worry - the issue may have been test only and resolved separately (in a change to the test case) - I don't think we're seeing cases in practice/production where an article doesn't have the cached_tag_list column set correctly.

@maestromac
Copy link
Member

image
my gut feeling based on this 14 days heatmap is it's a tolerable performance loss. The performance loss is not very noticeable.

@jgaskins
Copy link
Contributor

jgaskins commented Dec 7, 2021

Agreed with @maestromac. There are a few outliers but they're acceptable. Also, those outliers started a full day after that PR was merged, with thousands of reactions created in that time.

@djuber djuber closed this Dec 7, 2021
@djuber djuber deleted the revert-15638-djuber/use-tag-list-in-follow-worker branch January 20, 2022 00:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: draft bot applied label for PR's that are a work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants