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.

Sign up for GitHub

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

Jump to bottom

Eliminate some N+1s #155

Merged
merged 3 commits into from
Sep 2, 2024
Merged

Eliminate some N+1s #155

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1340d67
Eliminate some N+1s
joshuay03 Aug 30, 2024
820206d
Merge branch 'main' into perf-improvements
andresag4 Sep 2, 2024
62aef2c
Merge branch 'main' into perf-improvements
andresag4 Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,6 +625,7 @@ PLATFORMS
arm64-darwin-21
arm64-darwin-22
arm64-darwin-23
arm64-darwin-24
x86_64-linux

DEPENDENCIES
Expand Down
6 changes: 4 additions & 2 deletions app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,11 @@ class ApplicationController < ActionController::Base
def current_profile = current_user&.profile

# TODO: Must change after implementing multi-conference support
def current_conference = Conference.last
def current_conference
@_current_conference ||= Conference.last
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is called a bunch of times per request resulting in a query each time.

end

def vapid_public_key
Base64.urlsafe_decode64(ENV["VAPID_PUBLIC_KEY"]).bytes.to_json
@_vapid_public_key ||= Base64.urlsafe_decode64(ENV["VAPID_PUBLIC_KEY"]).bytes.to_json
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one isn't a big deal but since it's computed every time time main layout renders it seemed worth memoising as well.

end
end
2 changes: 1 addition & 1 deletion app/controllers/schedules_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ def show
@sessions = SessionQuery.new(
relation: current_user.sessions.where(conference: current_conference),
params: filter_params
).call.includes(:attendees, :location, :speakers, :tags).order(:starts_at)
).call.includes(:location, :tags, speakers: [profile: :image_attachment]).order(:starts_at)
Copy link
Contributor Author

@joshuay03 joshuay03 Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This comment applies to the two actions amended in SessionsController below as well.

The #attendees association isn't utilised in any subsequent views, so it's an unnecessary preload query. Images are queried per-speaker in sessions/show and sessions/_card which are N+1s.

end

private
Expand Down
4 changes: 2 additions & 2 deletions app/controllers/sessions_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ def index
@sessions = SessionQuery.new(
relation: sessions.joins(:location).distinct,
params: filter_params
).call.includes(:attendees, :tags).order(:starts_at)
).call.includes(:location, :tags, speakers: [profile: :image_attachment]).order(:starts_at)
end

def show
@session = sessions.friendly.find(params[:id])
@session = sessions.friendly.includes(:location, :tags, speakers: [profile: :image_attachment]).find(params[:id])
end

private
Expand Down
2 changes: 1 addition & 1 deletion app/views/layouts/_bottom_navbar.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
<% if unread_notifications.present? %>
<div class="absolute -top-0.5 right-0 w-[18px] h-[18px] rounded-full bg-red flex justify-center items-center">
<span class="text-[11px] text-white font-bold px-1">
<%= unread_notifications.count < 10 ? unread_notifications.count : "+9" %>
<%= unread_notifications.length < 10 ? unread_notifications.length : "+9" %>
</span>
</div>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/schedules/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
</a>
</div>
<div class="w-full mx-2 mb-1 border-b"></div>
<div class="mb-1 text-gray-400 text-nowrap"><%= pluralize(sessions.count, "Session") %></div>
<div class="mb-1 text-gray-400 text-nowrap"><%= pluralize(sessions.length, "Session") %></div>
Copy link
Contributor Author

@joshuay03 joshuay03 Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This comment applies to all similar changes below. Calling #count on a relation object will always execute a query, whereas #length is delegated to the records, which are loaded if they haven't been already, which in this case they have thanks to the preloads.

<% end %>
<% sessions.each do |session| %>
<%= render(
Expand Down
4 changes: 2 additions & 2 deletions app/views/sessions/_card.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
:div,
class: [
"flex flex-col ml-2",
"max-w-[200px]": session.speakers.count > 1
"max-w-[200px]": session.speakers.length > 1
]
) do %>
<h2 class="text-sm italic font-black group-hover:text-red group-focus:text-red"><%= speaker.name %></h2>
Expand All @@ -71,7 +71,7 @@
<% end %>

<div class="flex flex-col">
<% if session.tags.exists? %>
<% if session.tags.present? %>
Copy link
Contributor Author

@joshuay03 joshuay03 Aug 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Similar to the #count changes, #exists? will execute a query whereas #present? will be delegated to the preloaded records.

<div class="flex flex-wrap w-full gap-2 mb-3">
<% session.tags.each do |tag| %>
<div class="px-2 py-1 text-xs bg-transparent border rounded-lg border-bluegray-600 text-bluegray-600">
Expand Down
2 changes: 1 addition & 1 deletion app/views/sessions/index.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
</a>
</div>
<div class="w-full mx-2 mb-1 border-b"></div>
<div class="mb-1 text-gray-400 text-nowrap"><%= pluralize(sessions.count, "Session") %></div>
<div class="mb-1 text-gray-400 text-nowrap"><%= pluralize(sessions.length, "Session") %></div>
<% end %>
<% sessions.each do |session| %>
<%= render(
Expand Down
4 changes: 2 additions & 2 deletions app/views/sessions/show.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@

<%= content_tag(:div, class: [
"flex flex-col ml-2",
"max-w-[200px]": @session.speakers.count > 1
"max-w-[200px]": @session.speakers.length > 1
] ) do %>
<h2 class="text-sm italic font-black group-hover:text-red group-focus:text-red"><%= speaker.name %></h2>
<p class="text-sm font-normal text-gray-400"><%= speaker.job_title %></p>
Expand Down Expand Up @@ -94,7 +94,7 @@
<div class="flex flex-col items-center w-full p-5 mt-6">
<div class="flex flex-col items-start w-full max-w-screen-sm">
<p class="mb-4 text-2xl italic font-bold text-black">
<%= "About the #{'speaker'.pluralize(@session.speakers.count)}" %>
<%= "About the #{'speaker'.pluralize(@session.speakers.length)}" %>
</p>
<div class="flex flex-col w-full gap-4">
<% @session.speakers.each do |speaker| %>
Expand Down