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

optimization task (Lutovinova) #122

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kseni265
Copy link

@kseni265 kseni265 commented Mar 9, 2023

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!

@@ -0,0 +1,13 @@
FROM ruby:3.1.0-alpine
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice

@@ -59,7 +59,7 @@ Case-study должен получиться рассказом с технич

### Совет: как посчитать кол-во строк в файле
```
wc -l data_large.rb # (3250940) total line count
wc -l data_large.txt # (3250940) total line count
Copy link
Collaborator

Choose a reason for hiding this comment

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

справедливо, никто кажется раньше не замечал

## Формирование метрики
Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумала использовать такую метрику: время выполнения программы на 10000 строках данных.
Если мы хотим добиться чтобы на 3250940 строках в файле при линейной зависимости скорость обработки составляла не более 30 секунд,
то на 10000 время обработки должно быть менее чем ~90 миллисекунд, добьемся того чтобы на 10000 скрипт выполнялся не более 90 миллисекунд.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вообще очень логично и здраво

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

90мс маловато


### Ваша находка №5
Точка роста осталась на методе each но на этот раз на методе each внтури сбора информации по пользователям.
Вынесла этот кусок в отдельный метод, чтобы понять что именно увеличивает время. По отчету видно что дольше всего в цикле отрабатывает
Copy link
Collaborator

Choose a reason for hiding this comment

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

Супер-правильный ход с выносом анонимного блока в отдельный метод

```

При запуске бенчмарка оказалось что метрика неверна и предварительно подготовленные небольшие файлы отрабатывают больше чем за 30 секунд.
Получила новые замеры на предварительно оптимизированном коде и решила уменьшить метркиу до 45 ms. Т.о снова пытаемся оптимизировать код но уже по новой метрике
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше было бы взять файл побольше и ещё раз прикинуть за сколько он должен обработаться

Удалось улучшить метрику системы с 1.34 сек на 10000 строк до 35ms на 10000 строк и уложиться в заданный бюджет.

За время оптимизации попробовала многие профилировщики. Визуально более простым и понятным показался ruby-prof.
Cамым информативным в нетривиальных проблемах для меня оказался stackprof и ruby-prof с отчетам в виже callstack.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

uniqueBrowsers += [browser] if uniqueBrowsers.all? { |b| b != browser }
end

uniqueBrowsers = sessions.map { |s| s[:browser] }.sort.uniq
Copy link
Collaborator

Choose a reason for hiding this comment

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

Лучше для сбора уникальных браузеров использовать Set / SortedSet

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