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

[potashin] optimization #109

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

Conversation

potashin
Copy link

No description provided.

if nesting == 0 # если закончился объкет уровня trip, парсим и импортируем его
trip = FastJsonparser.parse(str)

copy(
Copy link
Author

@potashin potashin May 16, 2024

Choose a reason for hiding this comment

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

тут, конечно, забивается на валидации всех моделей совершенно.

идея по этому поводу была такая – использовать по одному инстансу модели на каждую и валидировать их перед копи/записью в справочники через валидации, которые не лезут в базу. на те, которые лезут в базу, вроде uniqueness, сделать уникальный индекс, потом делать reset шаблона.
тут есть момент по реализации, что делать с валидным родительским элементом, но невалидные дочерним (или наоборот).

второй вариант: переносить все валидации на констрейнты и индексы в базу

Copy link
Collaborator

Choose a reason for hiding this comment

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

справедливо; у меня кстати по поводу валидаций в моделях и констрейнтов в pg как-то не сложилось однозначного мнения

в целом валидации в модели имеют место быть - на мой взгляд, ответственность модели - это быть валидным кирпичиком предметной области;

но с другой стороны если валидации будут в базе - у модели тоже не получится быть невалидной

но с третьей стороны в моделях как-то попроще и нет очень много завязок на БД

короче путь компромиссов

@potashin potashin force-pushed the feature/potashin-optimization branch from 82002a5 to a899f61 Compare May 16, 2024 22:38
В результате проделанной оптимизации наконец удалось обработать файл с данными.
Удалось улучшить метрику системы с с 77 секунд до 1.4с для medium и уложиться в заданный бюджет.
Файл large грузится за 6.5 секунд
Файл 1м стал грузится за 56 секунд.
Copy link
Author

@potashin potashin May 16, 2024

Choose a reason for hiding this comment

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

тут хочется понять, как обработать файл на 10м до минуты (нужны наводки, сейчас грузит чуть меньше 10 минут). из идей пока только использовать sax parser вроде ::Oj::Saj.

Copy link
Collaborator

Choose a reason for hiding this comment

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

а я что-то потерял контекст; где-то было что за 1 минуту можно? (я просто не помню)

можно попрофилировать, посмотреть ;)

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

Copy link
Author

Choose a reason for hiding this comment

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

@spajic в ридми написано, что large должен до минуты проходить, я подумал, что для остальных такая же метрика, с учетом дальнейших оптимизаций))

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.

Всё круто, респект 💪

@@ -1 +1 @@
2.6.3
2.6.10
Copy link
Collaborator

Choose a reason for hiding this comment

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

ого, вот это им попатчить пришлось ))


gem 'rails', '~> 5.2.3'
gem 'pg', '>= 0.18', '< 2.0'
gem 'puma', '~> 3.11'
gem 'bootsnap', '>= 1.1.0', require: false
gem 'fast_jsonparser'
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.

возможно прикольно было бы к рельсам по дефолту его прикрутить; или сделать возможность настройки

типа чтобы где-то в кишках Rails.cache.read не терялось время на парсинг строки не самым эффективным


def initialize
@cities = {}
@buses = Hash.new { |h, k| h[k] = {} }
Copy link
Collaborator

Choose a reason for hiding this comment

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

классная штука с автоматической инициализацией

if nesting == 0 # если закончился объкет уровня trip, парсим и импортируем его
trip = FastJsonparser.parse(str)

copy(
Copy link
Collaborator

Choose a reason for hiding this comment

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

справедливо; у меня кстати по поводу валидаций в моделях и констрейнтов в pg как-то не сложилось однозначного мнения

в целом валидации в модели имеют место быть - на мой взгляд, ответственность модели - это быть валидным кирпичиком предметной области;

но с другой стороны если валидации будут в базе - у модели тоже не получится быть невалидной

но с третьей стороны в моделях как-то попроще и нет очень много завязок на БД

короче путь компромиссов

copy trips (from_id, to_id, start_time, duration_minutes, price_cents, bus_id) from stdin with csv delimiter ';'
SQL

ActiveRecord::Base.connection.raw_connection.copy_data(sql) do
Copy link
Collaborator

Choose a reason for hiding this comment

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

кайф, лайк за стриминг, очень мало кто это делает в этом задании

Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику: время загрузки medium файла (10к записей, 77 секунд в первой итерации)

## Гарантия корректности работы оптимизированной программы
Программа не поставлялась с тестом, поэтому перед выполнением оптимизации я добавил его самостоятельно: загрузка example файла с дальнейшим сравнением загруженных в бд данных с эталоном. Выполнение этого теста в фидбек-лупе позволяет не допустить изменения логики программы при оптимизации.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

В результате проделанной оптимизации наконец удалось обработать файл с данными.
Удалось улучшить метрику системы с с 77 секунд до 1.4с для medium и уложиться в заданный бюджет.
Файл large грузится за 6.5 секунд
Файл 1м стал грузится за 56 секунд.
Copy link
Collaborator

Choose a reason for hiding this comment

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

а я что-то потерял контекст; где-то было что за 1 минуту можно? (я просто не помню)

можно попрофилировать, посмотреть ;)

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

Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику: время загрузки страницы `автобусы/Самара/Москва` при наличии 100к поездок в базе данных. Начальное измерение – 13.3с.

## Гарантия корректности работы оптимизированной программы
Программа не поставлялась с тестом, поэтому перед выполнением оптимизации я добавил его самостоятельно: результат работы страницы `автобусы/Самара/Москва` для данных из файла `fixtures/example.json` сравнивается с тем, который был сформирован до изменений.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍


## Результаты
В результате проделанной оптимизации наконец удалось обработать файл с данными.
Удалось улучшить метрику системы с 13.3с до 0.6с.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

get(:index, params: {from: 'Самара', to: 'Москва'})

assert_response(:success)
assert_equal(@response.body.squish, File.read('test/fixtures/files/example_index.html').squish)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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.

None yet

2 participants