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 1 #125

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

Conversation

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

Понял, что мне неудобно постоянно запускать процесс, искать его pid и подключаться еще к нему.
При этом сам отчет показался слишком расплывчатым без точных деталей.

- Проверил быстродействие с помощью `ruby-prof`
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Тут вас не особо интересует быстродействие, а распределение времени по программе

### Ваша находка №1
- С помощью `stackprof` посмотрел быстродействие системы.
```
21 ( 72.4%) Array#each
Copy link
Collaborator

Choose a reason for hiding this comment

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

а) что-то пошло не так, это не самая большая проблема

б) посмотрел все места с each тоже неправильно, там несколько разных each, надо разбираться какой именно самый жирный

3 ( 12.5%) Array#each
```

Время исполнения снизилось с ~`3.673483s` до ~`2.516519s` для файла в 10000 строк
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вот время исполнения для файла из 10к строк это и есть метрика на самом деле (на этом этапе)

### Ваша находка №2
- С помощью `stackprof` speedscore посмотрел быстродействие системы.
```
1.54s (85%) 360.04ms (20%) Array#select
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вот это top-1 проблема на самом деле

- Вместо использования select перешел на проверку наличия слова `user` для определения строки, содержащей информацию о пользователе, последующие строки записывал в сессии этого пользователя
- Провел рефакторинг, вместо того, чтобы каждый раз делать `users.each` на каждое поле для вывода результата, сделал один `each` для сбора значний всех полей.
- Убрал излишний вызов `Date.parse(d)}.sort.reverse.map { |d| d.iso8601 }`. В нем не было никакого смысла из-за того, что дата и так была в нужном формате
- Ускорил конкатенацию строк в присваивании user_key, исходя из документации: https://github.com/fastruby/fast-ruby#string-concatenation-code
Copy link
Collaborator

Choose a reason for hiding this comment

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

Антипаттерн - много изменений на одном шаге - не понято что как сработало


### Заметки для себя
- Нужно смотреть не только на код, но и на постановку задачи и данные, которые нужно обработать
- Осторожнее добавляй в git большие файлы, из-за них потом не всегда получается залить код в репозиторий и пользоваться такими инструментами как https://git-lfs.com/
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{ 'longestSession' => user.sessions.map {|s| s['time']}.map {|t| t.to_i}.max.to_s + ' min.' }
end
def uniqueBrowsers
@uniqueBrowsers ||= @sessions.map { |s| s['browser'] }.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