Skip to content

Task 3 #119

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

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

Task 3 #119

wants to merge 8 commits into from

Conversation

elenachekhina
Copy link
Contributor

No description provided.

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.

Nice work!

def stream
Enumerator.new do |yielder|
loop do
yielder << Oj.load(object)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 ого, найс

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

Choose a reason for hiding this comment

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

👍 лайк за collection и spacer_template

@total_count = Trip.where(from: @from, to: @to).count
@trips = Trip.where(from: @from, to: @to)
.order(:start_time)
.limit(@per).offset(@per * (@page - 1))
Copy link
Collaborator

Choose a reason for hiding this comment

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

пагинация limit offset в целом имеет для меня плохую репутацию (вроде бы про это что-то было в лекциях)

Trip.where(from: @from, to: @to).count при большом кол-ве трипов может начать жёстко тормозить, что негативно скажется на времени загрузки страницы (у нас тут их так много нет, но сам паттерн такой пагинации потенциально проблемный)


Конечная метрика: время выполнения импорта файла `fixtures/large.json` должно укладываться в 60 сек.
Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумала использовать такую метрику:
1) обработка файла после внесенных оптимизаций должна быть меньше, чем до оптимизации
Copy link
Collaborator

Choose a reason for hiding this comment

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

сорри за духоту, но это не метрика; метрика это число какое-то, характеризующее твою систему. В данном случае ты наверно берёшь за метрику время загрузки файла маленького/среднего (чтобы не ждать large.json)


## Предварительная подготовка

1) Добавила в проект тесты
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

4) вынесла в класс DataLoader, перенесла тесты

Пока выносила обратила внимание что файл считывается в память целиком, также в задании упоминается про файл 1М который весит примерно 3ГБ, перепишем сразу на стриминг
добавила класс JsonStreamer - собирает объекты и возвращает по одному. Гемы потоковой обработки выглядят сложновато для достаточно простой задачи, возможно они работают быстрее, но этим можно будет озаботиться попозже
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

возможно сейчас какой-то гем и есть рабочий, но в целом это не серебрянная пуля

6) large: 3.958625
7) 1M: 38.381950
8) ну тут уже время упирается в стример, без каких либо преобразований он перебирает файл 1M за 34.973238
9) причем потребление памяти на 1М не превышает 8МБ
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 кайф-кайф

-> Parallel Seq Scan on trips (cost=0.00..23750.00 rows=36 width=34)
Filter: ((from_id = 10) AND (to_id = 92))
(4 rows)
4) Избавимся от Seq Scan добавив составной индекс. Так как поиск идет всегда по (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.

составной индекс лайк


## Защита от регрессии производительности
1) для импорта данных был написан тест на проверку времени выполнения
2) для отображения расписания был бы написан тест на N+1 запрос, если бы rspec-sqlimit был совместим с rails 8.0.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

))

@@ -0,0 +1,26 @@
# Mark existing migrations as safe
StrongMigrations.start_after = 20250222090128
Copy link
Collaborator

Choose a reason for hiding this comment

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

strong_migrations one love

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