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

merged 3 commits into from
Sep 2, 2024

Conversation

joshuay03
Copy link
Contributor

Description

Nice work with the app! I noticed some low hanging perf improvements that could help make things feel a bit snappier.

Some high level comparisons:

# http://127.0.0.1:3000/sessions?only_path=true&starts_at=2024-08-31
# Before:
#   Completed 200 OK in 338ms (Views: 180.5ms | ActiveRecord: 4.3ms (128 queries, 22 cached) | GC: 41.3ms)
# After:
#   Completed 200 OK in 228ms (Views: 66.2ms | ActiveRecord: 1.3ms (27 queries, 3 cached) | GC: 17.0ms)

# http://127.0.0.1:3000/sessions?only_path=true
# Before:
#   Completed 200 OK in 842ms (Views: 678.7ms | ActiveRecord: 11.5ms (246 queries, 40 cached) | GC: 304.7ms)
# After:
#   Completed 200 OK in 318ms (Views: 153.8ms | ActiveRecord: 2.2ms (40 queries, 3 cached) | GC: 29.0ms)

How has this been tested?

Please mark the tests that you ran to verify your changes. If difficult to test, consider providing instructions so reviewers can test.

  • Manual testing
  • System tests
  • Unit tests
  • None

Checklist

  • CI pipeline is passing
  • My code follows the conventions of this project
  • I have performed a self-review of my code
  • I have commented on my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation (if applicable)
  • I have added seed data to the database (if applicable)

joshuay03
joshuay03 commented Aug 30, 2024
View reviewed changes
app/controllers/application_controller.rb
@@ -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.

joshuay03
joshuay03 commented Aug 30, 2024
View reviewed changes
app/controllers/application_controller.rb

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.

joshuay03
joshuay03 commented Aug 30, 2024
View reviewed changes
app/controllers/schedules_controller.rb
@@ -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.

joshuay03
joshuay03 commented Aug 30, 2024
View reviewed changes
app/views/schedules/show.html.erb
@@ -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.

joshuay03
joshuay03 commented Aug 30, 2024
View reviewed changes
app/views/sessions/_card.html.erb
@@ -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.

JGTR
JGTR reviewed Aug 30, 2024
View reviewed changes
Copy link
Collaborator

@JGTR JGTR left a comment

Choose a reason for hiding this comment

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

Great improvements @joshuay03 . 👍

@andresag4 andresag4 merged commit e8f164c into TelosLabs:main Sep 2, 2024
4 checks passed
@joshuay03 joshuay03 deleted the perf-improvements branch September 2, 2024 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JGTR JGTR JGTR left review comments

@andresag4 andresag4 andresag4 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshuay03 @JGTR @andresag4