Use cleverer patterns to fetch/iterate AR objects when checking presence #5833

rhymes · 2020-02-01T19:07:36Z

What type of PR is this? (check all applicable)

Description

As you know AR has many "presence" methods attached to "has many" relations: .present?, .blank?, .any?, .empty?, .none?, .exists?

Apart from the obvious result we expect due to their names, these are not all intercheangeable, sometimes resulting in unnecessary queries.

Armed with the section "any?, exists? and present?" (I've already submit a PR to optimize counts #5478) in the article 3 ActiveRecord Mistakes That Slow Down Rails Apps: Count, Where and Present by Nate Berkopec I went around the code, replaced those that could be cleverer in how they fetch data, documented the why of some of them and made sure they made sense for our cases.

Please let me know if some do not make sense for you.

Related Tickets & Documents

A bit of a background, I'm going to quote Nate directly:

These six predicate methods, which are English-language synonyms all asking the same question, have completely different implementations and performance implications, and these consequences depend on which version of Rails you are using

Confusing eh? :D Fortunately there's a nice table in the article that summarizes what happens in Rails 5.1+:

method	SQL generated	memoized?	implementation	Runs query if loaded?
present?	SELECT “users”.* FROM “users”	yes (load)	Object (!blank?)	no
blank?	SELECT “users”.* FROM “users”	yes (load)	load; blank?	no
any?	SELECT 1 AS one FROM “users” LIMIT 1	no unless loaded	!empty?	no
empty?	SELECT 1 AS one FROM “users” LIMIT 1	no unless loaded	exists? if !loaded?	no
none?	SELECT 1 AS one FROM “users” LIMIT 1	no unless loaded	empty?	no
exists?	SELECT 1 AS one FROM “users” LIMIT 1	no	ActiveRecord::Calculations	yes

from https://www.speedshop.co/2019/01/10/three-activerecord-mistakes.html

… render users

…orgs

rhymes · 2020-02-01T19:09:12Z

app/services/users/delete_articles.rb

@@ -3,7 +3,7 @@ module DeleteArticles
    module_function

    def call(user, cache_buster = CacheBuster)
-      return unless user.articles.any?
+      return if user.articles.blank?


as we iterate with .map right after (which issues a select * anyway), this is going to issue one less query.

rhymes · 2020-02-01T19:09:40Z

app/views/additional_content_boxes/boxes.html.erb

@@ -6,7 +6,8 @@
             follow_cue: @boosted_article.organization&.tag_line || @boosted_article.organization&.tag_line %>
 <% end %>

-<% if @suggested_articles.any? %>
+<%# the pattern .present?/.each has the advantage of issuing only a single SQL query to load objects in memory %>


you'll notice the pattern .present?/.each many times in this PR

rhymes · 2020-02-01T19:10:07Z

app/views/articles/tags/_sidebar_additional.html.erb

@@ -1,14 +1,15 @@
 <div id="sidebar-wrapper-right" class="sidebar-wrapper sidebar-wrapper-right">
  <div class="sidebar-bg" id="sidebar-bg-right"></div>
  <div class="side-bar sidebar-additional showing" id="sidebar-additional">
-    <% if Article.active_threads([@tag, "discuss"], Timeframer.new(params[:timeframe]).datetime).any? %>
+    <% active_threads = Article.active_threads([@tag, "discuss"], Timeframer.new(params[:timeframe]).datetime) %>


this just made sense, we were calling .active_threads twice in the same partial

rhymes · 2020-02-01T19:10:45Z

app/views/partnerships/_form.html.erb

@@ -1,4 +1,4 @@
-<% if !organizations.any? %>
+<% if organizations.none? %>


if not any made less sense that if none ;)

rhymes · 2020-02-01T19:11:49Z

app/views/users/_main_feed.html.erb

@@ -8,11 +8,11 @@
 <% end %>
 <% @stories.each_with_index do |story, i| %>
  <%= render "articles/single_story", story: story %>
-  <% if i == 0 && @comments.any? %>
+  <% if i == 0 && @comments.present? %>


users/comment_section will definitely load comments, so we can preload them here, this, combined with the changes inside the partial, saves one or two queries

citizen428

Nice one @rhymes 👏

mstruve

I really think we should keep organization.users.find_each

mstruve · 2020-02-03T19:46:59Z

app/views/organizations/_sidebar_additional.html.erb

    <% if @organization.users.present? %>
      <div class="widget">
        <div class="widget-suggested-follows-container">
          <header><h4>meet the team</h4></header>
          <div class="widget-body">
-            <% @organization.users.find_each do |user| %>


I think this may happen sooner than you think. You get one large org added right now and this will break. I think I would prefer to stick with the safety of find_each rather than risking it for a small speed up from using .present?/each

I was wondering about this line yesterday, but then I decided that I like @rhymes change, since find_each's default batch size is 1000, and it seems very unlikely that we'd have an org with more users than that. And I guess it wouldn't break, "just" consume a lot of memory.

Given we have talked about setting up communities for large organizations, our first Github, Microsoft etc will have well over 1k users. As for breaking modes, this will either timeout in which case the page will fail to load OR if we do OOM there is a chance it takes the whole box down with it as I have not seen great memory slice control from Heroku

Ok let's revert it, there's a larger issue at stake here which is the fact that we shouldn't do these kind of queries in the views (actually we shouldn't do queries in the views as much as possible :D). I'm going to switch back to .find_each here

I saw that you re-requested my review but still seeing the each change here. Did you forget to push it up?

@mstruve oh shoot! yes :( done

Been to that rodeo before! 😝

@mstruve Thanks for the context, I wasn't aware of any plans to onboard organizations of this size.

One question regarding the failure modes: in order to render the page, we'll still need to finish all iterations of find_each, so if that takes too long we will still run into timeouts, right? This suggests that we should probably keep a mental note of this particular line and move the SQL queries out of the view as @rhymes suggested.

For sure but I think thats a project for another day

mstruve · 2020-02-03T19:49:02Z

app/views/users/_organizations_area.html.erb

 <% if @user.organizations.present? %>
  <div id="sidebar-organizations" class="widget">
    <div class="widget-suggested-follows-container">
      <header><h4>organizations</h4></header>
      <div class="widget-body">
-        <% @user.organizations.find_each do |organization| %>
+        <% @user.organizations.each do |organization| %>


Going this way I am fine with bc I dont think a user will likely every belong to thousands of orgs.

Ridhwana

These are nice optimisations @rhymes!

rhymes · 2020-02-04T23:10:56Z

Sorry @lightalloy and @Ridhwana for invalidating your reviews :(

I left out a change, see aca30c7

mstruve

LGTM 🚀

rhymes added 9 commits February 1, 2020 18:17

Use .any?/.find_each pattern for organizations/_sidebar_additional to…

bda2a00

… render users

Use .any?/.find_each pattern for users/_organizations_area to render …

d9dd7f7

…orgs

Replace .blank? with .none? in Notifications::Update.call

59757bf

Replace .any?/.find_each pattern with .present?/.each

0a39e62

Use .present?/.each where it makes sense

a482e19

Only load comments once in user's page

efba807

Replace more with smarter patterns

4caa617

Store stuff in variables and don't use instance variables

83f0ae3

More present?/each

aa8a8b2

rhymes requested review from jacobherrington and Zhao-Andy as code owners February 1, 2020 19:07

rhymes commented Feb 1, 2020

View reviewed changes

rhymes changed the title ~~[WIP] Use cleverer patterns to fetch/iterate AR objects when checking presence~~ Use cleverer patterns to fetch/iterate AR objects when checking presence Feb 1, 2020

pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Feb 1, 2020

rhymes requested review from lightalloy and mstruve and removed request for jacobherrington and Zhao-Andy February 1, 2020 19:26

citizen428 self-requested a review February 3, 2020 04:54

citizen428 previously approved these changes Feb 3, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 3, 2020

lightalloy previously approved these changes Feb 3, 2020

View reviewed changes

weekly-digest bot mentioned this pull request Feb 3, 2020

Weekly Digest (27 January, 2020 - 3 February, 2020) #5847

Closed

mstruve reviewed Feb 3, 2020

View reviewed changes

Switch to any?/find_each for organization's users

cf4fda6

rhymes dismissed stale reviews from lightalloy and citizen428 via cf4fda6 February 4, 2020 13:34

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 4, 2020

rhymes requested review from lightalloy and mstruve February 4, 2020 13:35

lightalloy previously approved these changes Feb 4, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 4, 2020

rhymes requested a review from Ridhwana February 4, 2020 17:09

Ridhwana previously approved these changes Feb 4, 2020

View reviewed changes

Switch to any?/find_each for organization's users, take 2

aca30c7

rhymes dismissed stale reviews from Ridhwana and lightalloy via aca30c7 February 4, 2020 23:09

pr-triage bot added PR: unreviewed bot applied label for PR's with no review and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 4, 2020

rhymes requested review from Ridhwana and lightalloy February 4, 2020 23:10

mstruve approved these changes Feb 5, 2020

View reviewed changes

pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 5, 2020

Ridhwana approved these changes Feb 5, 2020

View reviewed changes

lightalloy approved these changes Feb 5, 2020

View reviewed changes

mstruve merged commit a95877e into forem:master Feb 5, 2020

pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 5, 2020

rhymes deleted the rhymes/dont-present-ar-relations branch February 5, 2020 13:31

weekly-digest bot mentioned this pull request Feb 10, 2020

Weekly Digest (3 February, 2020 - 10 February, 2020) #5980

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cleverer patterns to fetch/iterate AR objects when checking presence #5833

Use cleverer patterns to fetch/iterate AR objects when checking presence #5833

rhymes commented Feb 1, 2020 •

edited

Loading

rhymes Feb 1, 2020

rhymes Feb 1, 2020

rhymes Feb 1, 2020

rhymes Feb 1, 2020

rhymes Feb 1, 2020

citizen428 left a comment

mstruve left a comment

mstruve Feb 3, 2020

citizen428 Feb 4, 2020 •

edited

Loading

mstruve Feb 4, 2020

rhymes Feb 4, 2020

mstruve Feb 4, 2020

rhymes Feb 4, 2020

mstruve Feb 5, 2020

citizen428 Feb 5, 2020

mstruve Feb 5, 2020

mstruve Feb 3, 2020

Ridhwana left a comment

rhymes commented Feb 4, 2020

mstruve left a comment

		@@ -1,4 +1,4 @@
		<% if !organizations.any? %>
		<% if organizations.none? %>

Use cleverer patterns to fetch/iterate AR objects when checking presence #5833

Use cleverer patterns to fetch/iterate AR objects when checking presence #5833

Conversation

rhymes commented Feb 1, 2020 • edited Loading

What type of PR is this? (check all applicable)

Description

Related Tickets & Documents

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

citizen428 left a comment

Choose a reason for hiding this comment

mstruve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

citizen428 Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ridhwana left a comment

Choose a reason for hiding this comment

rhymes commented Feb 4, 2020

mstruve left a comment

Choose a reason for hiding this comment

rhymes commented Feb 1, 2020 •

edited

Loading

citizen428 Feb 4, 2020 •

edited

Loading