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

Week 2 #127

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

Conversation

AlexeyRyabchikov
Copy link

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.

Approve, nice work, хорошо, что использовали фидбек от 1го ДЗ и хорошо, что поделали итераций до переделывания на потоковую работу 👍


## Формирование метрики
Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику: программа не должна потреблять больше **70Мб** памяти при обработке файла `data_large.txt` в течение всей своей работы.
(Учел ошибку первой домашней работы и сделал объем данных больше, для того чтобы проблемы были более очевидны на частичных файлах от основного)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Программа поставлялась с тестом. Выполнение этого теста в фидбек-лупе позволяет не допустить изменения логики программы при оптимизации.

## Feedback-Loop
Для того, чтобы иметь возможность быстро проверять гипотезы я выстроил эффективный `feedback-loop`, который позволил мне получать обратную связь по эффективности сделанных изменений за 15-20 секунд.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

Строка 103:
user_sessions = sessions.select { |session| session['user_id'] == user['id'] }
```
- Как и в первой домашней работе заменил перебор всех сессий на хэш с группированные данных по user_id. В данном конкретном месте алгоритмическая сложность с O(n) изменилась на O(1).
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


Строка 139:
collect_stats_from_users(report, users_objects) do |user|
{ 'dates' => user.sessions.map{|s| s['date']}.map {|d| Date.parse(d)}.sort.reverse.map { |d| d.iso8601 } }
Copy link
Collaborator

Choose a reason for hiding this comment

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

строка кстати избыточно плотная, тут и map 3 раза, ещё sort, ещё reverse, ещё Date.parse,...

для более понятных отчётов в таких случаях можно просто отформатировать на несколько строчек это


## Результаты
В результате проделанной оптимизации наконец удалось обработать файл с данными.
Удалось улучшить метрику системы с 131 MB при выполнении 20_000к строк и 226 MB при выполнении 40_000к строк в начале, до ~38 мБ на файле `data_large.txt` и уложиться в заданный бюджет.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@sessions = sessions
end
def parse_line(line)
type, id, *params = line.split(',')
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.

что-то я подозреваю, что *params может подтормаживать по сравнению с явным указанием переменных (не критично, просто заметка)

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