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

Task 2 #118

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

Task 2 #118

wants to merge 11 commits into from

Conversation

elenachekhina
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.

Very nice work! ✅

## Формирование метрики
Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику на каждой итерации оптимизации:
1) отчет профилировщика должен поменять главную точку роста
2) обработка файла должна ускориться / потребление памяти должно уменьшиться
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`, который позволил мне получать обратную связь по эффективности сделанных изменений за 10-15 секунд
Copy link
Collaborator

Choose a reason for hiding this comment

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

10-15 секунд супер

Вот какие проблемы удалось найти и решить

Для тренировки прошлась по исходному файлу и нашла места, где можно оптимизировать память
Написала runner, в котором запускается программа и второй поток, который мониторит потребляемую память и записывает ее в файл 1 раз в секунду
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 👍

MEMORY USAGE: 19 MB
MEMORY USAGE: 19 MB
MEMORY USAGE: 19 MB
FINAL MEMORY USAGE: 19 MB
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 👍

FINAL MEMORY USAGE: 19 MB
```

Но, если взять граничное значение в виде один юзер и все сессии (кол-во сессий как в файле large), то память конечно раздувается, вероятно из-за того что происходит накопление не уникальных браузеров / дат, и из-за этого время ухудшается до ~8 секунд
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice catch с краевым случаем!

{ 'usedIE' => user.sessions.map{|s| s['browser']}.any? { |b| b.upcase =~ /INTERNET EXPLORER/ } }
def save_user
stream.push_key("#{user.first_name} #{user.last_name}")
stream.push_object
Copy link
Collaborator

Choose a reason for hiding this comment

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

лайк за push_key, push_object, etc.. выглядит получше, чем собирать json совсем голыми руками

it 'uses under 20MB of memory' do
expect do
work('data/data.txt')
end.to perform_allocation(41_000).bytes
Copy link
Collaborator

Choose a reason for hiding this comment

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

не бьётся с названием спека

в принципе можно было бы просто тоже сделать замер RSS до и после спека и сделать ассерт на дельту

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