From 128a6411d2a60c855e3c99303d0157f436e32f13 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Mon, 25 Jan 2016 16:56:23 +0100 Subject: Don't pluck project IDs for events By instead using a sub-query we save ourselves the overhead of loading any data into memory only to pass it on to another query. --- app/controllers/dashboard/projects_controller.rb | 2 +- app/controllers/dashboard_controller.rb | 6 +++--- app/controllers/groups_controller.rb | 6 +----- app/models/event.rb | 6 +++++- app/models/project.rb | 2 ++ 5 files changed, 12 insertions(+), 10 deletions(-) diff --git a/app/controllers/dashboard/projects_controller.rb b/app/controllers/dashboard/projects_controller.rb index 58e9049f158..0b7fcdf5e9e 100644 --- a/app/controllers/dashboard/projects_controller.rb +++ b/app/controllers/dashboard/projects_controller.rb @@ -36,7 +36,7 @@ class Dashboard::ProjectsController < Dashboard::ApplicationController private def load_events - @events = Event.in_projects(@projects.pluck(:id)) + @events = Event.in_projects(@projects) @events = @event_filter.apply_filter(@events).with_associations @events = @events.limit(20).offset(params[:offset] || 0) end diff --git a/app/controllers/dashboard_controller.rb b/app/controllers/dashboard_controller.rb index 087da935087..139e40db180 100644 --- a/app/controllers/dashboard_controller.rb +++ b/app/controllers/dashboard_controller.rb @@ -23,14 +23,14 @@ class DashboardController < Dashboard::ApplicationController protected def load_events - project_ids = + projects = if params[:filter] == "starred" current_user.starred_projects else current_user.authorized_projects - end.pluck(:id) + end - @events = Event.in_projects(project_ids) + @events = Event.in_projects(projects) @events = @event_filter.apply_filter(@events).with_associations @events = @events.limit(20).offset(params[:offset] || 0) end diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index fb26a4e6fc3..7bbef5ab29c 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -87,10 +87,6 @@ class GroupsController < Groups::ApplicationController @projects ||= ProjectsFinder.new.execute(current_user, group: group).sorted_by_activity.non_archived end - def project_ids - @projects.pluck(:id) - end - # Dont allow unauthorized access to group def authorize_read_group! unless @group and (@projects.present? or can?(current_user, :read_group, @group)) @@ -123,7 +119,7 @@ class GroupsController < Groups::ApplicationController end def load_events - @events = Event.in_projects(project_ids) + @events = Event.in_projects(@projects) @events = event_filter.apply_filter(@events).with_associations @events = @events.limit(20).offset(params[:offset] || 0) end diff --git a/app/models/event.rb b/app/models/event.rb index 01d008035a5..8dfc1d04709 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -47,7 +47,11 @@ class Event < ActiveRecord::Base # Scopes scope :recent, -> { reorder(id: :desc) } scope :code_push, -> { where(action: PUSHED) } - scope :in_projects, ->(project_ids) { where(project_id: project_ids).recent } + + scope :in_projects, ->(projects) do + where(project_id: projects.reorder(nil).id_only).recent + end + scope :with_associations, -> { includes(project: :namespace) } scope :for_milestone_id, ->(milestone_id) { where(target_type: "Milestone", target_id: milestone_id) } diff --git a/app/models/project.rb b/app/models/project.rb index 4bd51449c25..63ecf5b7105 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -215,6 +215,8 @@ class Project < ActiveRecord::Base scope :public_and_internal_only, -> { where(visibility_level: Project.public_and_internal_levels) } scope :non_archived, -> { where(archived: false) } + scope :id_only, -> { select(:id) } + state_machine :import_status, initial: :none do event :import_start do transition [:none, :finished] => :started -- cgit v1.2.1 From de7c9c7ab17a8b36ca0c645ba1a7a6971a077fb1 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 26 Jan 2016 11:54:57 +0100 Subject: Use Atom update times of the first event By simply loading the first event from the already sorted set we save ourselves extra (slow) queries just to get the latest update timestamp. This removes the need for Event.latest_update_time and significantly reduces the time needed to build an Atom feed. Fixes gitlab-org/gitlab-ce#12415 --- app/models/event.rb | 8 +------- app/models/project.rb | 2 -- app/views/dashboard/projects/index.atom.builder | 2 +- app/views/groups/show.atom.builder | 2 +- app/views/projects/show.atom.builder | 2 +- app/views/users/show.atom.builder | 2 +- spec/models/event_spec.rb | 21 --------------------- 7 files changed, 5 insertions(+), 34 deletions(-) diff --git a/app/models/event.rb b/app/models/event.rb index 8dfc1d04709..4be23a1cf72 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -49,7 +49,7 @@ class Event < ActiveRecord::Base scope :code_push, -> { where(action: PUSHED) } scope :in_projects, ->(projects) do - where(project_id: projects.reorder(nil).id_only).recent + where(project_id: projects.select(:id).reorder(nil)).recent end scope :with_associations, -> { includes(project: :namespace) } @@ -68,12 +68,6 @@ class Event < ActiveRecord::Base [Event::CREATED, Event::CLOSED, Event::MERGED]) end - def latest_update_time - row = select(:updated_at, :project_id).reorder(id: :desc).take - - row ? row.updated_at : nil - end - def limit_recent(limit = 20, offset = nil) recent.limit(limit).offset(offset) end diff --git a/app/models/project.rb b/app/models/project.rb index 63ecf5b7105..4bd51449c25 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -215,8 +215,6 @@ class Project < ActiveRecord::Base scope :public_and_internal_only, -> { where(visibility_level: Project.public_and_internal_levels) } scope :non_archived, -> { where(archived: false) } - scope :id_only, -> { select(:id) } - state_machine :import_status, initial: :none do event :import_start do transition [:none, :finished] => :started diff --git a/app/views/dashboard/projects/index.atom.builder b/app/views/dashboard/projects/index.atom.builder index 2e2712c5146..d4daf07c6c0 100644 --- a/app/views/dashboard/projects/index.atom.builder +++ b/app/views/dashboard/projects/index.atom.builder @@ -4,7 +4,7 @@ xml.feed "xmlns" => "http://www.w3.org/2005/Atom", "xmlns:media" => "http://sear xml.link href: dashboard_projects_url(format: :atom, private_token: current_user.try(:private_token)), rel: "self", type: "application/atom+xml" xml.link href: dashboard_projects_url, rel: "alternate", type: "text/html" xml.id dashboard_projects_url - xml.updated @events.latest_update_time.xmlschema if @events.any? + xml.updated @events[0].updated_at.xmlschema if @events[0] @events.each do |event| event_to_atom(xml, event) diff --git a/app/views/groups/show.atom.builder b/app/views/groups/show.atom.builder index 5cc0f5e1d2e..c66b82bb484 100644 --- a/app/views/groups/show.atom.builder +++ b/app/views/groups/show.atom.builder @@ -4,7 +4,7 @@ xml.feed "xmlns" => "http://www.w3.org/2005/Atom", "xmlns:media" => "http://sear xml.link href: group_url(@group, format: :atom, private_token: current_user.try(:private_token)), rel: "self", type: "application/atom+xml" xml.link href: group_url(@group), rel: "alternate", type: "text/html" xml.id group_url(@group) - xml.updated @events.latest_update_time.xmlschema if @events.any? + xml.updated @events[0].updated_at.xmlschema if @events[0] @events.each do |event| event_to_atom(xml, event) diff --git a/app/views/projects/show.atom.builder b/app/views/projects/show.atom.builder index d6762219108..2468509242a 100644 --- a/app/views/projects/show.atom.builder +++ b/app/views/projects/show.atom.builder @@ -4,7 +4,7 @@ xml.feed "xmlns" => "http://www.w3.org/2005/Atom", "xmlns:media" => "http://sear xml.link href: namespace_project_url(@project.namespace, @project, format: :atom, private_token: current_user.try(:private_token)), rel: "self", type: "application/atom+xml" xml.link href: namespace_project_url(@project.namespace, @project), rel: "alternate", type: "text/html" xml.id namespace_project_url(@project.namespace, @project) - xml.updated @events.latest_update_time.xmlschema if @events.any? + xml.updated @events[0].updated_at.xmlschema if @events[0? @events.each do |event| event_to_atom(xml, event) diff --git a/app/views/users/show.atom.builder b/app/views/users/show.atom.builder index 114d1e7a379..e9e466c6350 100644 --- a/app/views/users/show.atom.builder +++ b/app/views/users/show.atom.builder @@ -4,7 +4,7 @@ xml.feed "xmlns" => "http://www.w3.org/2005/Atom", "xmlns:media" => "http://sear xml.link href: user_url(@user, :atom), rel: "self", type: "application/atom+xml" xml.link href: user_url(@user), rel: "alternate", type: "text/html" xml.id user_url(@user) - xml.updated @events.latest_update_time.xmlschema if @events.any? + xml.updated @events[0].updated_at.xmlschema if @events[0] @events.each do |event| event_to_atom(xml, event) diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 071582b0282..ec2a923f91b 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -65,27 +65,6 @@ describe Event, models: true do it { expect(@event.author).to eq(@user) } end - describe '.latest_update_time' do - describe 'when events are present' do - let(:time) { Time.utc(2015, 1, 1) } - - before do - create(:closed_issue_event, updated_at: time) - create(:closed_issue_event, updated_at: time + 5) - end - - it 'returns the latest update time' do - expect(Event.latest_update_time).to eq(time + 5) - end - end - - describe 'when no events exist' do - it 'returns nil' do - expect(Event.latest_update_time).to be_nil - end - end - end - describe '.limit_recent' do let!(:event1) { create(:closed_issue_event) } let!(:event2) { create(:closed_issue_event) } -- cgit v1.2.1