-
Notifications
You must be signed in to change notification settings - Fork 24
optimized version and tests #3
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
base: master
Are you sure you want to change the base?
Conversation
spajic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аппрув + несколько комментов.
Было бы хорошо более конкретно расписывать шаги оптимизации.
Каким инструментом нашли самую жирную проблему, в чём была причина, как её исправили, как повлияло на метрику.
| Вот как я построил `feedback_loop`: | ||
| 1. Изначально выбрал маленький размер исходных данных (около 1Мб) позволяющий скрипту успешно отработать без оптимизаций | ||
| 2. Поиск базовой метрики (время и память) | ||
| 3. Дописал тест на регрессию по времени и памяти |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| ### Ваша находка №1 | ||
| О вашей находке №1 | ||
| ### Находка №1 | ||
| Считывание всего файла в строку с дальнейшим разбиением на массив строк крайне неэффективно и было одним из основным блокеров для работы с большими файлами |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Опечатка одним из основных
| ### Ваша находка №1 | ||
| О вашей находке №1 | ||
| ### Находка №1 | ||
| Считывание всего файла в строку с дальнейшим разбиением на массив строк крайне неэффективно и было одним из основным блокеров для работы с большими файлами |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Как именно это обнаружили?
| ### Находка №2 | ||
| Memory_profiler показал, что аллоцируется огромное количество строк, хоть большинство из них собираются GC, однако все равно увеличивают объем потребляемой памяти и время выполнения (как минимум за счет работы GC) | ||
|
|
||
| Для исправления этой проблемы был использован ```# frozen_string_literal: true``` и в качестве ключей стали использоваться Symbol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Насколько сильно помогло?
Как повлияло на метрики?
| ### Находка №6 | ||
| Использование "медленных" методов без особой надобности | ||
|
|
||
| Ruby-prof показал, что метод Date#parse занимает порядка 8% от всего времени выполнения, поэтому он был заменен на Date#strptime |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| Удалось улучшить метрику системы с *того, что у вас было в начале, до того, что получилось в конце* | ||
|
|
||
| *Какими ещё результами можете поделиться* | ||
| В среднем файл обрабатывается за 29 - 30 секунд при средних затратах по памяти 850 - 860Мб |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| @@ -1,60 +1,50 @@ | |||
| # Deoptimized version of homework task | |||
| # frozen_string_literal: true | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.