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.

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

Perform optimization #110

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ gem 'rails', '~> 5.2.3'
gem 'pg', '>= 0.18', '< 2.0'
gem 'puma', '~> 3.11'
gem 'bootsnap', '>= 1.1.0', require: false
gem 'oj'
gem 'activerecord-import'
gem 'rack-mini-profiler'

group :development, :test do
# Call 'byebug' anywhere in the code to stop execution and get a debugger console
Expand Down
14 changes: 12 additions & 2 deletions Gemfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ GEM
activemodel (= 5.2.3)
activesupport (= 5.2.3)
arel (>= 9.0)
activerecord-import (1.6.0)
activerecord (>= 4.2)
activestorage (5.2.3)
actionpack (= 5.2.3)
activerecord (= 5.2.3)
Expand All @@ -51,7 +53,7 @@ GEM
concurrent-ruby (1.1.5)
crass (1.0.4)
erubi (1.8.0)
ffi (1.10.0)
ffi (1.15.5)
globalid (0.4.2)
activesupport (>= 4.2.0)
i18n (1.6.0)
Expand All @@ -68,17 +70,22 @@ GEM
marcel (0.3.3)
mimemagic (~> 0.3.2)
method_source (0.9.2)
mimemagic (0.3.3)
mimemagic (0.3.10)
nokogiri (~> 1)
rake
mini_mime (1.0.1)
mini_portile2 (2.4.0)
minitest (5.11.3)
msgpack (1.2.9)
nio4r (2.3.1)
nokogiri (1.10.2)
mini_portile2 (~> 2.4.0)
oj (3.13.23)
pg (1.1.4)
puma (3.12.1)
rack (2.0.6)
rack-mini-profiler (3.1.1)
rack (>= 1.2.0)
rack-test (1.1.0)
rack (>= 1.0, < 3)
rails (5.2.3)
Expand Down Expand Up @@ -134,11 +141,14 @@ PLATFORMS
ruby

DEPENDENCIES
activerecord-import
bootsnap (>= 1.1.0)
byebug
listen (>= 3.0.5, < 3.2)
oj
pg (>= 0.18, < 2.0)
puma (~> 3.11)
rack-mini-profiler
rails (~> 5.2.3)
tzinfo-data
web-console (>= 3.3.0)
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/trips_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ class TripsController < ApplicationController
def index
@from = City.find_by_name!(params[:from])
@to = City.find_by_name!(params[:to])
@trips = Trip.where(from: @from, to: @to).order(:start_time)
@trips = Trip.where(from: @from, to: @to).preload(bus: :services).order(:start_time)
end
end
6 changes: 6 additions & 0 deletions app/models/buses_service.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# frozen_string_literal: true

class BusesService < ApplicationRecord
belongs_to :bus
belongs_to :service
end
113 changes: 113 additions & 0 deletions app/services/tasks/import_json_data.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
# frozen_string_literal: true

module Tasks
class ImportJsonData
Copy link
Collaborator

Choose a reason for hiding this comment

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

лайк за сервис

BATCH_SIZE = 1000

attr_accessor :cities, :trips, :services, :buses_hash, :buses_array, :buses_services, :cities_hash
attr_reader :filepath

class << self
def call(filepath:)
new(filepath).call
end
end

def initialize(filepath)
@filepath = filepath
@cities = []
@cities_hash = {}
@trips = []
@services = {}
@buses_hash = {}
@buses_array = []
@buses_services = []
end

def call
ActiveRecord::Base.transaction do
create_services

File.open(filepath) do |ff|
nesting = 0
str = +''

until ff.eof?
ch = ff.read(1) # читаем по одному символу
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 лайк за стриминг, мало кто делает

if ch == '{' # начинается объект, повышается вложенность
nesting += 1
str << ch
elsif ch == '}' # заканчивается объект, понижается вложенность
nesting -= 1
str << ch
if nesting.zero? # если закончился объкет уровня trip, парсим и импортируем его
trip = Oj.load(str)
process_trip(trip)
str = +''
end
elsif nesting >= 1
str << ch
end
end
end
end
end

private

def create_services
Service::SERVICES.each do |service|
services[service] = Service.create!(name: service).id
end
end

def process_trip(trip)
bus_key = "#{trip['bus']['number']}-#{trip['bus']['model']}"

trip['bus']['services'].each do |service|
buses_services << [bus_key, services[service]]
end

buses_array << [trip['bus']['number'], trip['bus']['model']]

cities << [trip['from']]
cities << [trip['to']]

trips << [trip['from'], trip['to'], trip['start_time'], trip['duration_minutes'], trip['price_cents'], bus_key]

if trips.size == BATCH_SIZE
City.import %i[name], cities.uniq!, on_duplicate_key_ignore: true
update_cities_hash(cities)

Bus.import %i[number model], buses_array, on_duplicate_key_ignore: true
update_bus_hash(buses_array)

trips.map! do |trip|
[cities_hash[trip[0]], cities_hash[trip[1]], trip[2], trip[3], trip[4], buses_hash[trip[5]]]
end
Trip.import %i[from_id to_id start_time duration_minutes price_cents bus_id], trips

buses_services.map! { |buses_service| [buses_hash[buses_service.first], buses_service.last] }
BusesService.import %i[bus_id service_id], buses_services, on_duplicate_key_ignore: true

@buses_array = []
@trips = []
@cities = []
@buses_services = []
end
end

def update_bus_hash(buses_array)
buses_array.uniq.each do |bus|
key = "#{bus.first}-#{bus.last}"
buses_hash[*key] = Bus.find_by(number: bus.first, model: bus.last).id unless buses_hash[*key]
end
end

def update_cities_hash(cities)
cities.each do |city|
cities_hash[*city] = City.find_by(name: city).id unless cities_hash[*city]
end
end
end
end
1 change: 0 additions & 1 deletion app/views/trips/_delimiter.html.erb

This file was deleted.

6 changes: 0 additions & 6 deletions app/views/trips/_services.html.erb

This file was deleted.

11 changes: 11 additions & 0 deletions app/views/trips/_trip.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,16 @@
<ul>
<li><%= "Отправление: #{trip.start_time}" %></li>
<li><%= "Прибытие: #{(Time.parse(trip.start_time) + trip.duration_minutes.minutes).strftime('%H:%M')}" %></li>
<li><%= "В пути: #{trip.duration_minutes / 60}ч. #{trip.duration_minutes % 60}мин." %></li>
<li><%= "Цена: #{trip.price_cents / 100}р. #{trip.price_cents % 100}коп." %></li>
<li><%= "Автобус: #{trip.bus.model} №#{trip.bus.number}" %></li>

<% if trip.bus.services.present? %>
<li>Сервисы в автобусе:</li>
<ul>
<%= render partial: "service", collection: trip.bus.services %>
</ul>
<% end %>
</ul>

====================================================
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вот этот разделитель можно тоже параметром задать (забавный факт: https://guides.rubyonrails.org/layouts_and_rendering.html#spacer-templates)

12 changes: 2 additions & 10 deletions app/views/trips/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,7 @@
<%= "Автобусы #{@from.name} – #{@to.name}" %>
</h1>
<h2>
<%= "В расписании #{@trips.count} рейсов" %>
<%= "В расписании #{@trips.length} рейсов" %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

лично я за size )

</h2>

<% @trips.each do |trip| %>
<ul>
<%= render "trip", trip: trip %>
<% if trip.bus.services.present? %>
<%= render "services", services: trip.bus.services %>
<% end %>
</ul>
<%= render "delimiter" %>
<% end %>
<%= render :partial => "trip", :collection => @trips %>
60 changes: 60 additions & 0 deletions case-study.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
## Задача 1: Импорт данных
Нужно оптимизировать механизм перезагрузки расписания из файла так, чтобы он импортировал файл _large.json_ в БД posgresql в пределах минуты

# Ход выполнения
Сразу было видно, даже без профилировщиков, что под каждую запись в бд делается отдельный INSERT запрос - это занимает основную часть времени.
Нужно применить подход с BATCH INSERT. Для этого использовался гем **activerecord-import**. Основная проблема была импортировать связи, т.к
требуется внешний ключ, т.е знание id непосредственно на уровне бд. Чтобы не делать лишних SELECT запросов, использовал хранение id автобусов, городов и сервисов в хеше на уровне оперативной памяти (хеш обновлялся каждый раз при добавлении новых объектов в базу)

Также добавлял индексы (в том числе и составные) на уникальность, т.к при BATCH INSERT возможно появления дубликатов и нужна валидация на уровне бд
Copy link
Collaborator

Choose a reason for hiding this comment

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

кстати говоря, индексы лучше в таком случае накидывать после импорта

может иметь смысл даже такой флоу

  • удалили индекс
  • импортнули данные
  • создали индекс (если не создался - увы, что-то не так пошло с импортом)

зависит от использования конечно; но поинт в том, что вставки в базе идут быстрее когда не приходится индекс обновлять / проверять / поддерживать


# Результат
Для оценки скорости работы рейк таски использовал библиотеку Benchmark

```
bundle exec rake reload_json[fixtures/large.json]

Total time is: 21.044087463000324
```

-> получилось добиться импорта файла _large.json_ за +- 20 секунд


## Задача 2: Отображение расписаний
Сами страницы расписаний тоже формируются не эффективно и при росте объёмов начинают сильно тормозить.

Нужно найти и устранить проблемы, замедляющие формирование этих страниц

Предварительно импортирован файл _large.json_

# Ход выполнения
Для оценки скорости работы загрузки страницы использовал гем **rack-mini-profiler**

### Находка №1
- видна проблема n+1 при прогрузке ассоциаций в index.html.erb `trip.bus.services`
- Добавил `preload` в контроллер
- рендер страницы ускорился с 5 секунд до 2.7 секунд

### Находка №2
- видна долгая загрузка partial _trip.html.erb и _service.html.erb
- удалил конструкцию с `@trips.each` и переписал на более быстрый подход рендера с конструкцией `render partial: "trip", collection: @trips`
- аналогично для сервисов: `<%= render partial: "service", collection: trip.bus.services %>`
- рендер страницы ускорился с 2.7 секунд до 500мс
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


### Находка №3
- Делался лишний запрос `SELECT COUNT(*)` - исправил вызов `.count` на `.length` на @trips

### Находка №4
- Рендер вьюх всё ещё занимал основное время
- Дропнул ненужную вьюху `_delimiter.html.erb` и просто вставил текст из неё в родительскую вьюху `_trip.html.erb`
- рендер страницы ускорился с 500мс до 260мс
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


# Результат
Для оценки скорости смотрел девелопмент лог и вывод гема rack-mini-profiler

```
Completed 200 OK in 263ms (Views: 235.8ms | ActiveRecord: 25.0ms)

```

-> получилось добиться открытия страницы за 263мс
Copy link
Collaborator

Choose a reason for hiding this comment

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

там ещё индексы на таблицы можно создать было бы; с точки зрения времени загрузки это не топ проблема, но с точки зрения нагрузки на базу - да; и это идёт как use-case pg-hero, так как он прям пишет какие индексы надо создать

2 changes: 2 additions & 0 deletions config/database.yml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ default: &default
# For details on connection pooling, see Rails configuration guide
# http://guides.rubyonrails.org/configuring.html#database-pooling
pool: <%= ENV.fetch("RAILS_MAX_THREADS") { 5 } %>
username: task-4_development
password: new_password

development:
<<: *default
Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20240514210140_add_uniqueness_constraint_to_bus.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddUniquenessConstraintToBus < ActiveRecord::Migration[5.2]
def change
add_index :buses, [:number, :model], unique: true
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddUniquenessConstraintToServices < ActiveRecord::Migration[5.2]
def change
add_index :services, :name, unique: true
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddUniquenessConstraintToCities < ActiveRecord::Migration[5.2]
def change
add_index :cities, :name, unique: true
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddUniquenessConstraintToBusesService < ActiveRecord::Migration[5.2]
def change
add_index :buses_services, [:bus_id, :service_id], unique: true
end
end
6 changes: 5 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,27 +10,31 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema.define(version: 2019_03_30_193044) do
ActiveRecord::Schema.define(version: 2024_05_15_233732) do

# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"

create_table "buses", force: :cascade do |t|
t.string "number"
t.string "model"
t.index ["number", "model"], name: "index_buses_on_number_and_model", unique: true
end

create_table "buses_services", force: :cascade do |t|
t.integer "bus_id"
t.integer "service_id"
t.index ["bus_id", "service_id"], name: "index_buses_services_on_bus_id_and_service_id", unique: true
end

create_table "cities", force: :cascade do |t|
t.string "name"
t.index ["name"], name: "index_cities_on_name", unique: true
end

create_table "services", force: :cascade do |t|
t.string "name"
t.index ["name"], name: "index_services_on_name", unique: true
end

create_table "trips", force: :cascade do |t|
Expand Down
Loading