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

Optimize db queries on the front page #420

Merged
merged 7 commits into from
Jun 20, 2018
Merged

Optimize db queries on the front page #420

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a05b771
optimize db queries on home page
BatedUrGonnaDie Jun 19, 2018
ae4815f
add .present? check to better indicate what is happening
BatedUrGonnaDie Jun 19, 2018
5d16e0d
don't always query first user rivals
BatedUrGonnaDie Jun 19, 2018
e17be3f
somehow i removed the 'i'
BatedUrGonnaDie Jun 19, 2018
0709c17
use runs instead of Run.where
BatedUrGonnaDie Jun 20, 2018
7c3c69f
Undo rubocop thing
BatedUrGonnaDie Jun 20, 2018
374782f
make rivalry code infinitely easier
BatedUrGonnaDie Jun 20, 2018
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ gem 'cancancan'
gem 'doorkeeper'

# db
gem 'active_record_union'
Copy link
Owner

Choose a reason for hiding this comment

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

Wow nice, hadn't heard of this before. This seems sick, and can definitely clean up a lot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, the other spot I noticed that had something similar was the game search, where we find game IDs from both games and game aliases. It seemed useful for the long term for some more advanced things.

gem 'activerecord-import'
gem 'arel'
gem 'aws-sdk-rails'
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ GEM
activemodel (>= 4.1, < 6)
case_transform (>= 0.2)
jsonapi-renderer (>= 0.1.1.beta1, < 0.3)
active_record_union (1.3.0)
activerecord (>= 4.0)
activejob (5.2.0)
activesupport (= 5.2.0)
globalid (>= 0.3.6)
Expand Down Expand Up @@ -436,6 +438,7 @@ PLATFORMS

DEPENDENCIES
active_model_serializers
active_record_union
activerecord-import
administrate
api-pagination (= 4.7)
Expand Down
4 changes: 2 additions & 2 deletions app/models/game.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ class Game < ApplicationRecord

validates :name, presence: true

has_many :categories
has_many :categories, dependent: :destroy
has_many :runs, through: :categories
has_many :users, -> { distinct }, through: :runs
has_many :runners, -> { distinct }, through: :runs, class_name: 'User'
has_many :aliases, class_name: 'GameAlias'
has_many :aliases, class_name: 'GameAlias', dependent: :destroy
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, I actually can't think of any current or future use case for destroying a game other than the existing merge flow, and in that case any aliases and categories should have already been ported over to the new game. It sounds a bit weird in theory, but realistically I'd consider the destroying of a game that still has aliases or categories to be a bug, so maybe these should be :restrict_with_exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This broke the tests, and since it was only supposed to be a rubocop fix, I'll leave this for another time.


after_create :create_initial_alias

Expand Down
16 changes: 10 additions & 6 deletions app/models/user.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class User < ApplicationRecord
include RivalUser
include TwitchUser

has_many :runs
has_many :runs, dependent: :destroy
Copy link
Owner

Choose a reason for hiding this comment

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

We give users the option to keep their runs when deleting:

screenshot 2018-06-19 10 02 06

The code that handles this does nothing to the runs if the user chooses to anonymize them, which seems wrong now that I'm looking at it. So should probably be

  • :nullify if we want to keep that code as-is, or
  • :restrict_with_exception if we want to fix that code to disown all runs, since in that situation, trying to destroy a user with any runs would be a bug since they all should have already been destroyed or disowned.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll revert this one, and make an issue for this. It should probably be restricted with exception though since nullify doesn't make sense if the user wants all runs deleted, so the code that handles this should raise an error if some are left.

has_many :categories, -> { distinct }, through: :runs
has_many :games, -> { distinct }, through: :runs

Expand Down Expand Up @@ -39,9 +39,9 @@ def self.search(term)

# avatar returns the user's avatar, or a default avatar if the user has not set one. It cannot return nil.
def avatar
return "https://splits.io/logo.svg" if read_attribute(:avatar).nil?
return 'https://splits.io/logo.svg' if self[:avatar].nil?
Copy link
Owner

Choose a reason for hiding this comment

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

Looks like self[:avatar] will error if nil, see: rubocop/rails-style-guide#155

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rubocop made it sound identical and I was trying to squash a warning, so I'll just revert this.


URI.parse(read_attribute(:avatar) || '').tap do |uri|
URI.parse(self[:avatar] || '').tap do |uri|
uri.scheme = 'https'
end.to_s
end
Expand All @@ -63,9 +63,8 @@ def previous_upload_for(category)
end

def pbs
runs.where(
id: categories.map { |category| pb_for(category).id10 } | runs.where(category: nil).pluck(:id)
)
Run.where(user: self).select('DISTINCT ON (category_id) *').order('category_id, realtime_duration_ms ASC')
Copy link
Owner

Choose a reason for hiding this comment

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

Any reason for using Run.where(user: self) here over runs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To be honest, when I started testing stuff I was working with joins which needs to be done on User, and then got stuck in that mindset. I'll take a look and see if this and the one below are equivalent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doing it like you said, i was able to add an extra where clause to make it so there will not be any duplicates, and then use union_all which doesn't have a de-duplication step which has a bit better performance.

.union(Run.where(user: self, category: nil))
Copy link
Owner

Choose a reason for hiding this comment

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

Same question as above, also there's the runs.by_category(nil) scope available if that is equivalent to this

end

def runs?(category)
Expand All @@ -76,6 +75,11 @@ def to_s
twitch_display_name || name || 'somebody'
end

def rivals
User.joins('INNER JOIN rivalries ON rivalries.to_user_id = users.id AND rivalries.from_user_id = ' + id.to_s)
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of string concatenation, use SQL interpolation like

User.joins('INNER JOIN ... AND rivalries.from_user_id = ?', id)

which will sanitize inputs to prevent SQL injection. It's just IDs in this case, but it's still a good practice (and who knows whether there might be a separate attack in the future to inject or set custom IDs).

.select('users.*, rivalries.category_id')
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of this select?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To have access to category_id in the returned relation object. Since rivalries aren't global like twitch follows, they only make sense within the context of a category. Otherwise we would get back a list of users and have no way to tell which user goes to which run. This way we get back a list of all users (which might have more than one of the same user), and then be able to iterate on them and figure out what category to then look up easily.

Copy link
Owner

Choose a reason for hiding this comment

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

Ok I'm confused. I understand the need to correlate a User with a Category, but doesn't this User#rivals function return an array of ActiveRecord Users? But later I see you calling both .category_id (a Rivalry method) and .pb_for (a User method) on them. What type of object are these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It returns an array of Users that have the category_id that they are a rival in embeded in them. That is why we do the weird select statement .select('users.*, rivalries.category_id'). It selects all fields from the user, and the category_id field from the rivalries table and merges it into one record.

Copy link
Owner

Choose a reason for hiding this comment

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

Whoa. I had no idea that was a thing Rails could do. It's like a... frankenstein object. Not sure how I feel about that, lol. Is it possible / still efficient to pass around Rivalrys as an alternative?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this pr, it never loads any of the Rivalry objects into memory, it just loads your rival user frankenstein objects. It could probably have user.rivals return a tuple of the User and Rival objects, with only one more db request (one to actually retrieve all rivals), and then zip them with the users (just remove the select statement from current code). I'm not sure how clean that would be though. Up to you on what you think is best.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, nevermind I'm just retarded. Adds one sql query but is much cleaner.

Copy link
Owner

Choose a reason for hiding this comment

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

lol ok nice

end

def twitch_followed_users
User.joins(:twitch_user_followers).where(twitch_user_follows: {from_user_id: id})
end
Expand Down
4 changes: 2 additions & 2 deletions app/views/runs/index.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
span Splits I/O
- else
h2 Splits I/O
- if logged_in? && current_user.runs.present?
- if logged_in? && current_user.runs.any?
h5 Want some jelly with these PBs?
- else
h5
Expand All @@ -20,7 +20,7 @@ article
.row
.col-lg-12
- if logged_in?
- if current_user.runs.present?
- if current_user.runs.any?
.card.shadow
= render partial: 'shared/run_table', locals: { \
runs: current_user.pbs, \
Expand Down
8 changes: 5 additions & 3 deletions app/views/shared/_follows.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@
h4 Recent PBs By People You Follow
.card.col-md-6.p-0
.list-group
- Run.includes(:game, :category, :user).where(user: current_user.twitch_followed_users).where.not(games: {name: [nil, ""]}).order('runs.created_at DESC').limit(10).each do |run|
- followed_runs = Run.includes(:game, :category, :user).where(user: current_user.twitch_followed_users).where.not(games: {name: [nil, ""]}).order('runs.created_at DESC').limit(10)
- user_runs = current_user.pbs.where(category_id: followed_runs.map(&:category_id))
- followed_runs.each do |run|
= link_to(run, class: 'list-group-item list-group-item-action flex-column align-items-start')
.media
= image_tag(run.user.avatar, size: '70x70', class: 'mr-3')
Expand All @@ -14,8 +16,8 @@
= "#{run.game} #{run.category}"
div
= format_ms(run.duration_ms(Run::REAL))
- if current_user.runs?(run.category)
- user_pb = current_user.pb_for(run.category)
- user_pb = user_runs.detect { |r| r.category_id == run.category_id }
- if user_pb.present?
= " ("
= pretty_difference(user_pb.duration_ms(Run::REAL), run.duration_ms(Run::REAL))
= ")"
8 changes: 4 additions & 4 deletions app/views/shared/_run_table.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
- description ||= nil
- cols ||= [:runner, :time, :name, :rival, :uploaded]
- runs = order_runs(runs).page(params[:page])
- if runs.empty?
- if runs.blank?
center
i No runs matched!
- else
Expand All @@ -24,7 +24,7 @@
- if cols.include?(:uploaded)
th.align-right = th_sorter('Uploaded', 'created_at')
tbody
- runs.includes(:user).each do |run|
- runs.includes(:user, :game, :category).each do |run|
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When ctrl+f'ing the project, every single call to the partial had the name col, so it seemed fine to just include these always.

tr id="run_#{run.id36}"
- if cols.include?(:owner_controls)
td.align-right
Expand All @@ -43,8 +43,8 @@
- if cols.include?(:rival)
td.align-right
small
- rival = run.user.rival_for(run.category)
- if rival.present? && rival.runs?(run.category)
- rival = run.user.rivals.detect { |r| r.category_id == run.category_id }
Copy link
Owner

Choose a reason for hiding this comment

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

Is this better because it caches the run.user.rivals query for use in all table rows?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, it caches the rivals call for every loop. I'm not sure if it caches the array that it turns it into though, so if you wanted maximum performance you could call rivals once at the top, and then use that local variable for each loop iteration.

This still requires an extra call to get the rivals run, just like before, but we eliminate that first call for every run.

- if rival.present? && rival.pb_for(run.category).present?
- rival_run = rival.pb_for(run.category)
= link_to compare_path(run, rival_run), class: 'run-link stealth-link' do
= pretty_difference(run.duration_ms(Run::REAL), rival_run.duration_ms(Run::REAL))
Expand Down