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

Задание 3 выполненное #17

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

sadgb
Copy link

@sadgb sadgb commented Feb 18, 2020

Время импорта large.json 30 секунд
Время отображения маршрутов 160ms без паджинации, 22мс с паджинацией по 20 записей

@sadgb sadgb requested a review from spajic February 18, 2020 22:40
Copy link
Collaborator

@spajic spajic left a comment

Choose a reason for hiding this comment

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

👍 Спасибо за работу!

@@ -1,7 +1,14 @@
class TripsController < ApplicationController
def index

#
# да этому здесь не место, но будет интересно посмотреть будет ли за это замечание ))
Copy link
Collaborator

Choose a reason for hiding this comment

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

))

@@ -13,8 +13,9 @@ class Bus < ApplicationRecord
].freeze

has_many :trips
has_and_belongs_to_many :services, join_table: :buses_services
has_many :buses_services
has_many :services, through: :buses_services
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -1,5 +1,5 @@
class City < ApplicationRecord
validates :name, presence: true, uniqueness: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@@ -29,4 +29,31 @@ def to_h
},
}
end


def self.get_data(from_id, to_id)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше бы в query-object вынести

</ul>
<%= render "delimiter" %>
<% end %>
<%= render partial: 'trip', collection: @trips %>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Плюс за рендеринг коллекции! Многие просто удаляют partial и вставляют его сюда


Поехали разбираться
- Select count(*) from trips where это нам легко убрать заменив @trips.count на длину массива @trips.load.length
Ничто не выполняется быстрее чем отсутствие запроса тут можно даже не тестировать
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Planning Time: 0.171 ms
Execution Time: 41.573 ms

Привет Seq Scan
Copy link
Collaborator

Choose a reason for hiding this comment

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

👋

Итого аж в 24 раза в production environment но выводить все трипы в одну страницу это все равно неправильно так как создает огромную нагрузку на браузер пользователя а не только на сервер
Поэтому нужна паджинация. Простейший вывод по 20 записей позволит получить нам сумасшедшие

Avg: .02172 что еще в 8 раз быстрее ( итого 190+ раз оптимизация )
Copy link
Collaborator

Choose a reason for hiding this comment

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

💪 💪


## Защита от регрессии производительности

Для защиты от регресии я написал rspec тест который ограничивает N+1и создание лишних запросов
Copy link
Collaborator

Choose a reason for hiding this comment

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

Супер!


## Защита от регрессии производительности

Дабы такое не повторялось я написал тест проверяющий производительность
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants