From 9124219f54c28efd19d19276e45e63a28f3d8f15 Mon Sep 17 00:00:00 2001 From: Anna Selivanova Date: Wed, 6 Mar 2019 00:11:59 +0400 Subject: [PATCH] Refactor & optimize script --- .ruby-version | 1 + case-study-template.md | 186 ++++++++++++++++++++++---- files/fixtures/data.txt | 18 +++ files/fixtures/expected_report.json | 1 + task-1.rb | 199 ++++++++++------------------ 5 files changed, 250 insertions(+), 155 deletions(-) create mode 100644 .ruby-version create mode 100644 files/fixtures/data.txt create mode 100644 files/fixtures/expected_report.json diff --git a/.ruby-version b/.ruby-version new file mode 100644 index 0000000..aedc15b --- /dev/null +++ b/.ruby-version @@ -0,0 +1 @@ +2.5.3 diff --git a/case-study-template.md b/case-study-template.md index e0eef00..78aef2e 100644 --- a/case-study-template.md +++ b/case-study-template.md @@ -12,35 +12,175 @@ Я решил исправить эту проблему, оптимизировав эту программу. ## Формирование метрики -Для того, чтобы понимать, дают ли мои изменения положительный эффект на быстродействие программы я придумал использовать такую метрику: *тут ваша метрика* +Я попыталась проанализировать, как зависит время выполнения скрипта от объема обрабатываемых данных, замеряя его на файлах разного размера. +На рисунке ниже показана зависимость: +![chart](https://ucarecdn.com/ebcafac8-0bc3-4813-8493-55bf36ff977f/execution_time.png) + + +К сожалению, такого кол-во данных недостаточно, чтобы сделать вывод о характере зависимости (мне не хватило терпения сделать больше замеров). +Для простоты я решила пока допустить, что она линейная. И что максимальное время, за которое должен отрабатывать скрипт на целевом файле (~ 135Mb), – 300 секунд. +Тогда мы можем выбрать метрику: `скорость обработки текста объемом 1Мб` и целевой показатель для неё: `2 секунды`. ## Гарантия корректности работы оптимизированной программы Программа поставлялась с тестом. Выполнение этого теста позволяет не допустить изменения логики программы при оптимизации. ## Feedback-Loop -Для того, чтобы иметь возможность быстро проверять гипотезы я выстроил эффективный `feedback-loop`, который позволил мне получать обратную связь по эффективности сделанных изменений за *время, которое у вас получилось* - -Вот как я построил `feedback_loop`: *как вы построили feedback_loop* - -## Вникаем в детали системы, чтобы найти 20% точек роста -Для того, чтобы найти "точки роста" для оптимизации я воспользовался *инструментами, которыми вы воспользовались* - -Вот какие проблемы удалось найти и решить - -### Ваша находка №1 -О вашей находке №1 - -### Ваша находка №2 -О вашей находке №2 - -### Ваша находка №X -О вашей находке №X +Для того, чтобы иметь возможность быстро проверять гипотезы я выстроил эффективный `feedback-loop`, который позволил мне получать обратную связь по эффективности сделанных изменений за время, не превышающее полминуты. + +Вот как я построила `feedback-loop`: + * Выделила из большого файла кусок объемом примерно 26_000 строк (~ 1Mb); + * Время выполнения скрипта на исходной версии кода составило 28-30 секунд, что вполне терпимо по скорости обратной связи; + * Для оценки времени использовала стандартный модуль Benchmark; + * Полученное время – моя целевая метрика; + * Следом за скриптом выполняется тест, чтобы проверить работоспособность кода после внесенных изменений. + +## Инструменты +Для того чтобы найти "точки роста" для оптимизации я воспользовалась: + * Гем memory_profiler + * Гем get_process_mem + * Гем ruby-prof + +## Обнаруженные проблемы и проведенные оптимизации +_Прим.: там где я ссылаюсь на строки кода, имеется в виду номер строки в исходном неоптимизированном скрипте._ + +### Оптимизация №1 +С помощью `get_process_mem` удалось выяснить, что больше всего потребление памяти возрастает после выполнения цикла по строкам, +в котором формируются массивы пользователей и сессий (`users`, `sessions`): с 30 до 230Mb. +Отчет `memory_profiler` так же показал, что больше всего памяти выделяется в строке 55, где идет формирование массива сессий. +В каждой итерации создаются новые объекты массивов – массив с новым элементом, массив с результатом конкатенации, это неэффективно. +Замена конкатенации на модификацию массивов `users` и `sessions` 'in place' с помощью `Array#push` снизило выделенную на эту +операцию память (после выполнения цикла занято оказалось около 50Mb), однако на итоговую скорость работы это особенно не повлияло. + +### Оптимизация №2 +Следующее место, на которое указывает `memory_profiler`, – строка 101, в которой идет выбор сессий пользователя с +помощью метода `#select`. Очевидно, можно значительно сэкономить ресурсы, если вместо массива `sessions` сразу же из +файла считывать данные в хэш, где ключом будет `id` пользователя, а значением – массив с сессиями. +Это изменение невозможно сделать, не порефакторив места, в которых используется массив `sessions`: + * он используется для подсчета `uniqueBrowsersCount`, + * `totalSessions` в итоговом объекте отчёта + * для формирования списка браузеров (который по факту совпадает с `uniqueBrowsers`). +Мы можем сделать всё это в том же цикле, где будем формировать хэш сессий. Т.о. не только избавимся от большого массива, +но и от всех лишних итераций по нему. + +``` +users = [] +sessions_by_users = {} +unique_browsers = [] +total_sessions = 0 + +file_lines.each do |line| + cols = line.split(',') + users << parse_user(line) if cols[0] == 'user' + next unless cols[1] == 'session' + + session = parse_session(line) + sessions_by_users[session['user_id']] ||= [] + sessions_by_users[session['user_id']] << session + browser = session['browser'].upcase + unique_browsers << browser unless unique_browsers.include?(browser) + total_sessions += 1 +end +``` + +**Результат: скрипт на файле 1Мб выполнился за 0.6 сек.** + +### Оптимизация №3 +Дальше с помощью того же `memory_profiler` я обратила внимание на чудовищное кол-во объектов, под которые выделяется память в +процессе работы скрипта (даже после предыдущих двух пунктов). Total allocated: 1_221_129 objects – раз в 50 больше, +чем строк в обрабатываемом файле. Больше всего из них – массивы, далее – строки. Я последовательно прошлась по самым +заметным моментам, не замеряя каждое микродействие: + * Строка 103: меняем конкатенацию на `Array#push`, чтобы избавиться от лишних массивов. + * Строка 53: видно, что мы разбиваем строку на массив `line.split(',')` только для того чтобы узнать, +что лежит у нее в первой "колонке", и потом никак его не используем; выбрасываем! меняем на метод `String#start_with?`; +строки `user`, `session` фризим, выносим в константы. + * Меняем ключи хэшей со строк на символы. + * Строки и регулярные выражения вынесем в константы и зафризим. + +**Результат: кол-во объектов уменьшилось в 1.5 раза. Объем памяти не превышает 90Mb. Скрипт выполнился за 0.4 сек.** + +### Оптимизация №4 +Обратила внимание на `allocated memory by location`, на первом месте код из строки 140 – обработка дат в массиве сессий +пользователя. +Во-первых, видим чейн из двух `#map`, потом сортировка, `reverse` и снова `map`. Каждый из методов создает новые массивы в памяти. +Кроме того, `RubyProf` показал, что большую часть времени тратится на вызов `#parse`. +Замена на `#strptime` оказалась примерно в три раза быстрее. + +Далее видно, что в этом месте последовательно вызывается метод +`collect_stats_from_users` (7 раз!), который итерирует по массиву объектов пользователей и выдергивает из них данные для отчёта. +5/7 вызовов этого метода присутствуют в топ-10 отчета `allocated memory`. Очевидно, что можно доставать всю инфу +за один проход по этому массиву, сэкономить на повторном использовании одних и тех же данных (например, выделенные +массивы браузеров, времени визитов) + не создавать лишних хэшей в памяти (#merge). + +``` +users_objects.each do |user| + user_key = "#{user.attributes[:first_name]} #{user.attributes[:last_name]}" + sessions_duration = user.sessions.map { |s| s[:time].to_i } + browsers = user.sessions.map { |s| s[:browser].upcase } + + report[:usersStats][user_key] = { + sessionsCount: user.sessions.count, + totalTime: "#{sessions_duration.sum} min.", + longestSession: "#{sessions_duration.max} min.", + browsers: browsers.sort.join(DELIMITER), + usedIE: browsers.any? { |b| b =~ IE_PATTERN }, + alwaysUsedChrome: browsers.all? { |b| b =~ CHROME_PATTERN }, + dates: user.sessions.map { |s| Date.strptime(s[:date], '%Y-%m-%d') }.sort.reverse.map!(&:iso8601) + } +end +``` + +**Итоговое время: 0.35 сек.** + +### Оптимизация №5 +После фикса предыдущего места лидером по медленности стал цикл `each` по массиву `users_objects`. +Во-первых, на этом шаге стало видно, что вообще-то от массива объектов `users_objects` никакого особо смысла нет, +можно работать прямо с массивом хэшей `users`, минуя создание лишних объектов. + +Во-вторых, параллельно с тем, что мы держим в памяти `users_objects` и хэш с сессиями `sessions_by_users` +мы формируем объект отчёта `report`, куда копируются данные из первых двух, в результате чего потребление памяти растет. + +Попробуем итерировать по `users` не с помощью `each`, а в цикле `while` и выкидывать отработанные записи. +Кроме того, можно так же удалять отработанные данные из хэша сессий пользователей (забирать значение с помощью `Hash#delete`). + +``` +until users.empty? + user = users.shift + user_key = "#{user[:first_name]} #{user[:last_name]}" + user_sessions = sessions_by_users.delete(user[:id]) || [] + sessions_duration = user_sessions.map { |s| s[:time].to_i } + browsers = user_sessions.map { |s| s[:browser] } + + report[:usersStats][user_key] = { + sessionsCount: user_sessions.count, + totalTime: "#{sessions_duration.sum} min.", + longestSession: "#{sessions_duration.max} min.", + browsers: browsers.sort.join(DELIMITER), + usedIE: browsers.any? { |b| b =~ IE_PATTERN }, + alwaysUsedChrome: browsers.all? { |b| b =~ CHROME_PATTERN }, + dates: user_sessions.map { |s| Date.strptime(s[:date], '%Y-%m-%d') }.sort.reverse.map!(&:iso8601) + } +end +``` + +**Итоговое время: 0.3 сек.** + +После этого я запустила скрипт на исходном файле. Он отработал за **50 сек**. +В принципе, выглядит неплохо, и можно было бы остановиться. +Проблема в том, что при чтении всего файла в массив резко возрастает объем памяти (более 500Mb), +мы попадаем в ситуацию `memory bloat`, и в случае файла еще бОльшего размера файла это может привести к ошибкам. +Поэтому добавила след. пункт. + +### Оптимизация №6 +Заменить чтение всего файла в память сразу на построчное чтение. +В результате потребление памяти снизилось на те самые 500Mb, а график роста стал более пологий. +К сожалению, время обработки большого файла выросло при этом с 50 до 52 секунд, но я решила этой разницей пренебречь. ## Результаты -В результате проделанной оптимизации наконец удалось обработать файл с данными. -Удалось улучшить метрику системы с *того, что у вас было в начале, до того, что получилось в конце* - -*Какими ещё результами можете поделиться* +В результате проделанной оптимизации наконец удалось обработать файл с данными. +Удалось улучшить метрику системы с 28-30 секунд до 0.3 сек, т.е. на два порядка. ## Защита от регресса производительности -Для защиты от потери достигнутого прогресса при дальнейших изменениях программы сделано *то, что вы для этого сделали* +Для защиты от потери достигнутого прогресса при дальнейших изменениях программы нужно зафиксировать метрику в значении, +например, 0.5 секунд (с запасом). +Добавить автоматический тест, который прогоняет скрипт с тестовыми данными размером 1Mb, и падает, если по времени не уложился. +На CI добавить. Я не знаю, как правильно пишутся такие тесты, но вижу, что существуют решения https://github.com/piotrmurach/rspec-benchmark diff --git a/files/fixtures/data.txt b/files/fixtures/data.txt new file mode 100644 index 0000000..393b0b8 --- /dev/null +++ b/files/fixtures/data.txt @@ -0,0 +1,18 @@ +user,0,Leida,Cira,0 +session,0,0,Safari 29,87,2016-10-23 +session,0,1,Firefox 12,118,2017-02-27 +session,0,2,Internet Explorer 28,31,2017-03-28 +session,0,3,Internet Explorer 28,109,2016-09-15 +session,0,4,Safari 39,104,2017-09-27 +session,0,5,Internet Explorer 35,6,2016-09-01 +user,1,Palmer,Katrina,65 +session,1,0,Safari 17,12,2016-10-21 +session,1,1,Firefox 32,3,2016-12-20 +session,1,2,Chrome 6,59,2016-11-11 +session,1,3,Internet Explorer 10,28,2017-04-29 +session,1,4,Chrome 13,116,2016-12-28 +user,2,Gregory,Santos,86 +session,2,0,Chrome 35,6,2018-09-21 +session,2,1,Safari 49,85,2017-05-22 +session,2,2,Firefox 47,17,2018-02-02 +session,2,3,Chrome 20,84,2016-11-25 diff --git a/files/fixtures/expected_report.json b/files/fixtures/expected_report.json new file mode 100644 index 0000000..ad48563 --- /dev/null +++ b/files/fixtures/expected_report.json @@ -0,0 +1 @@ +{"totalUsers":3,"uniqueBrowsersCount":14,"totalSessions":15,"allBrowsers":"CHROME 13,CHROME 20,CHROME 35,CHROME 6,FIREFOX 12,FIREFOX 32,FIREFOX 47,INTERNET EXPLORER 10,INTERNET EXPLORER 28,INTERNET EXPLORER 35,SAFARI 17,SAFARI 29,SAFARI 39,SAFARI 49","usersStats":{"Leida Cira":{"sessionsCount":6,"totalTime":"455 min.","longestSession":"118 min.","browsers":"FIREFOX 12, INTERNET EXPLORER 28, INTERNET EXPLORER 28, INTERNET EXPLORER 35, SAFARI 29, SAFARI 39","usedIE":true,"alwaysUsedChrome":false,"dates":["2017-09-27","2017-03-28","2017-02-27","2016-10-23","2016-09-15","2016-09-01"]},"Palmer Katrina":{"sessionsCount":5,"totalTime":"218 min.","longestSession":"116 min.","browsers":"CHROME 13, CHROME 6, FIREFOX 32, INTERNET EXPLORER 10, SAFARI 17","usedIE":true,"alwaysUsedChrome":false,"dates":["2017-04-29","2016-12-28","2016-12-20","2016-11-11","2016-10-21"]},"Gregory Santos":{"sessionsCount":4,"totalTime":"192 min.","longestSession":"85 min.","browsers":"CHROME 20, CHROME 35, FIREFOX 47, SAFARI 49","usedIE":false,"alwaysUsedChrome":false,"dates":["2018-09-21","2018-02-02","2017-05-22","2016-11-25"]}}} diff --git a/task-1.rb b/task-1.rb index 778672d..2e0e233 100644 --- a/task-1.rb +++ b/task-1.rb @@ -1,58 +1,50 @@ -# Deoptimized version of homework task - require 'json' -require 'pry' require 'date' require 'minitest/autorun' +require 'benchmark' -class User - attr_reader :attributes, :sessions - - def initialize(attributes:, sessions:) - @attributes = attributes - @sessions = sessions - end -end +IE_PATTERN = /^INTERNET EXPLORER/.freeze +CHROME_PATTERN = /^CHROME/.freeze +COMMA = ','.freeze +DELIMITER = ', '.freeze +USER_ROW_MARK = 'user'.freeze +SESSION_ROW_MARK = 'session'.freeze def parse_user(user) - fields = user.split(',') - parsed_result = { - 'id' => fields[1], - 'first_name' => fields[2], - 'last_name' => fields[3], - 'age' => fields[4], + fields = user.split(COMMA) + { + id: fields[1], + full_name: "#{fields[2]} #{fields[3]}" } end def parse_session(session) - fields = session.split(',') - parsed_result = { - 'user_id' => fields[1], - 'session_id' => fields[2], - 'browser' => fields[3], - 'time' => fields[4], - 'date' => fields[5], + fields = session.split(COMMA) + { + user_id: fields[1], + session_id: fields[2], + browser: fields[3], + time: fields[4], + date: fields[5] } end -def collect_stats_from_users(report, users_objects, &block) - users_objects.each do |user| - user_key = "#{user.attributes['first_name']}" + ' ' + "#{user.attributes['last_name']}" - report['usersStats'][user_key] ||= {} - report['usersStats'][user_key] = report['usersStats'][user_key].merge(block.call(user)) - end -end - -def work - file_lines = File.read('data.txt').split("\n") - +def create_report(source_file, target_file) users = [] - sessions = [] - - file_lines.each do |line| - cols = line.split(',') - users = users + [parse_user(line)] if cols[0] == 'user' - sessions = sessions + [parse_session(line)] if cols[0] == 'session' + sessions_by_users = {} + unique_browsers = [] + total_sessions = 0 + + File.open(source_file, 'r').each do |line| + users << parse_user(line) if line.start_with?(USER_ROW_MARK) + next unless line.start_with?(SESSION_ROW_MARK) + + session = parse_session(line) + sessions_by_users[session[:user_id]] ||= [] + sessions_by_users[session[:user_id]] << session + browser = session[:browser].upcase! + unique_browsers << browser unless unique_browsers.include?(browser) + total_sessions += 1 end # Отчёт в json @@ -73,104 +65,47 @@ def work report = {} report[:totalUsers] = users.count - - # Подсчёт количества уникальных браузеров - uniqueBrowsers = [] - sessions.each do |session| - browser = session['browser'] - uniqueBrowsers += [browser] if uniqueBrowsers.all? { |b| b != browser } - end - - report['uniqueBrowsersCount'] = uniqueBrowsers.count - - report['totalSessions'] = sessions.count - - report['allBrowsers'] = - sessions - .map { |s| s['browser'] } - .map { |b| b.upcase } - .sort - .uniq - .join(',') - - # Статистика по пользователям - users_objects = [] - - users.each do |user| - attributes = user - user_sessions = sessions.select { |session| session['user_id'] == user['id'] } - user_object = User.new(attributes: attributes, sessions: user_sessions) - users_objects = users_objects + [user_object] + report[:uniqueBrowsersCount] = unique_browsers.count + report[:totalSessions] = total_sessions + report[:allBrowsers] = unique_browsers.sort!.join(COMMA) + report[:usersStats] = {} + + until users.empty? + user = users.shift + user_sessions = sessions_by_users.delete(user[:id]) || [] + sessions_duration = user_sessions.map { |s| s[:time].to_i } + browsers = user_sessions.map { |s| s[:browser] } + + report[:usersStats][user[:full_name]] = { + sessionsCount: user_sessions.count, + totalTime: "#{sessions_duration.sum} min.", + longestSession: "#{sessions_duration.max} min.", + browsers: browsers.sort!.join(DELIMITER), + usedIE: browsers.any? { |b| b =~ IE_PATTERN }, + alwaysUsedChrome: browsers.all? { |b| b =~ CHROME_PATTERN }, + dates: user_sessions.map { |s| Date.strptime(s[:date], '%Y-%m-%d') }.sort!.reverse!.map!(&:iso8601) + } end - report['usersStats'] = {} - - # Собираем количество сессий по пользователям - collect_stats_from_users(report, users_objects) do |user| - { 'sessionsCount' => user.sessions.count } - end - - # Собираем количество времени по пользователям - collect_stats_from_users(report, users_objects) do |user| - { 'totalTime' => user.sessions.map {|s| s['time']}.map {|t| t.to_i}.sum.to_s + ' min.' } - end - - # Выбираем самую длинную сессию пользователя - collect_stats_from_users(report, users_objects) do |user| - { 'longestSession' => user.sessions.map {|s| s['time']}.map {|t| t.to_i}.max.to_s + ' min.' } - end - - # Браузеры пользователя через запятую - collect_stats_from_users(report, users_objects) do |user| - { 'browsers' => user.sessions.map {|s| s['browser']}.map {|b| b.upcase}.sort.join(', ') } - end - - # Хоть раз использовал IE? - collect_stats_from_users(report, users_objects) do |user| - { 'usedIE' => user.sessions.map{|s| s['browser']}.any? { |b| b.upcase =~ /INTERNET EXPLORER/ } } - end + File.write(target_file, "#{report.to_json}\n") +end - # Всегда использовал только Chrome? - collect_stats_from_users(report, users_objects) do |user| - { 'alwaysUsedChrome' => user.sessions.map{|s| s['browser']}.all? { |b| b.upcase =~ /CHROME/ } } - end +time = Benchmark.realtime do + create_report('data_large.txt', 'result.json') +end +puts "Execution time: #{time.round(2)}" - # Даты сессий через запятую в обратном порядке в формате iso8601 - collect_stats_from_users(report, users_objects) do |user| - { 'dates' => user.sessions.map{|s| s['date']}.map {|d| Date.parse(d)}.sort.reverse.map { |d| d.iso8601 } } - end +class TaskTest < Minitest::Test + def test_result + create_report('files/fixtures/data.txt', 'files/fixtures/result.json') - File.write('result.json', "#{report.to_json}\n") -end + expected_report = File.read('files/fixtures/expected_report.json') + actual_report = File.read('files/fixtures/result.json') -class TestMe < Minitest::Test - def setup - File.write('result.json', '') - File.write('data.txt', -'user,0,Leida,Cira,0 -session,0,0,Safari 29,87,2016-10-23 -session,0,1,Firefox 12,118,2017-02-27 -session,0,2,Internet Explorer 28,31,2017-03-28 -session,0,3,Internet Explorer 28,109,2016-09-15 -session,0,4,Safari 39,104,2017-09-27 -session,0,5,Internet Explorer 35,6,2016-09-01 -user,1,Palmer,Katrina,65 -session,1,0,Safari 17,12,2016-10-21 -session,1,1,Firefox 32,3,2016-12-20 -session,1,2,Chrome 6,59,2016-11-11 -session,1,3,Internet Explorer 10,28,2017-04-29 -session,1,4,Chrome 13,116,2016-12-28 -user,2,Gregory,Santos,86 -session,2,0,Chrome 35,6,2018-09-21 -session,2,1,Safari 49,85,2017-05-22 -session,2,2,Firefox 47,17,2018-02-02 -session,2,3,Chrome 20,84,2016-11-25 -') + assert_equal expected_report, actual_report end - def test_result - work - expected_result = '{"totalUsers":3,"uniqueBrowsersCount":14,"totalSessions":15,"allBrowsers":"CHROME 13,CHROME 20,CHROME 35,CHROME 6,FIREFOX 12,FIREFOX 32,FIREFOX 47,INTERNET EXPLORER 10,INTERNET EXPLORER 28,INTERNET EXPLORER 35,SAFARI 17,SAFARI 29,SAFARI 39,SAFARI 49","usersStats":{"Leida Cira":{"sessionsCount":6,"totalTime":"455 min.","longestSession":"118 min.","browsers":"FIREFOX 12, INTERNET EXPLORER 28, INTERNET EXPLORER 28, INTERNET EXPLORER 35, SAFARI 29, SAFARI 39","usedIE":true,"alwaysUsedChrome":false,"dates":["2017-09-27","2017-03-28","2017-02-27","2016-10-23","2016-09-15","2016-09-01"]},"Palmer Katrina":{"sessionsCount":5,"totalTime":"218 min.","longestSession":"116 min.","browsers":"CHROME 13, CHROME 6, FIREFOX 32, INTERNET EXPLORER 10, SAFARI 17","usedIE":true,"alwaysUsedChrome":false,"dates":["2017-04-29","2016-12-28","2016-12-20","2016-11-11","2016-10-21"]},"Gregory Santos":{"sessionsCount":4,"totalTime":"192 min.","longestSession":"85 min.","browsers":"CHROME 20, CHROME 35, FIREFOX 47, SAFARI 49","usedIE":false,"alwaysUsedChrome":false,"dates":["2018-09-21","2018-02-02","2017-05-22","2016-11-25"]}}}' + "\n" - assert_equal expected_result, File.read('result.json') + def teardown + File.unlink('files/fixtures/result.json') end end