Говорить о красивом коде легко. Давайте поговорим о неприятном коде и посмотрим, что мы можем из него извлечь.
Фон
Продукт, над которым я работал, имеет концепцию Account. На данном Account может быть один-ко-многим Teams, к которому принадлежат пользователи. Был RESTful API, который позволял запрашивать различные настройки Account и Team объектов формы.
/api/v1/features/team-<id>
Например, /api/v1/features/team-21 вернет все функции группы 21. Также была форма конечной точки, которая возвращала все команды и их соответствующие настройки: /api/v1/features/team-*
Эта проблема
Вкратце, в некоторых учетных записях с большим количеством команд запрос team-21 был медленным. Запрос team-* был исключительно медленным - для учетной записи с 500 командами ответ легко мог бы занять от десяти до пятнадцати секунд.
Давай профиль!
Моя первоначальная интуиция была такой: кто-то, вероятно, застрял в цикле запроса к базе данных или что-то в этом роде, и я исправил бы это простым изменением, сделав единственный вызов базы данных. Чтобы подтвердить эту гипотезу, я сделал быстрый профиль.
Этот код был написан на Python, поэтому cProfile был быстрым способом получить некоторые тайминги для производственной учетной записи. Звонок tocProfile.run('features = _inner_fetch_all_features("team-*", account_id)') дал мне результаты. И результаты были очень и очень неожиданными.
Первый профиль вернулся через 11,935 секунды. Первое, что бросилось в глаза, это то, что этот код провел необычайное количество времени в каком-то классе с именем AttrDict:
ncalls tottime percall cumtime percall filename:lineno(function)
...
11825 0.049 0.000 0.060 0.000 attrdict.py:26(__getattr__)
384912/432 1.762 0.000 7.012 0.016 attrdict.py:39(deepcopy)
454032/64800 0.650 0.000 6.943 0.000 attrdict.py:49(<genexpr>)
23650 0.023 0.000 0.023 0.000 attrdict.py:58(__setstate__)
… примерно 59% всего времени было на AttrDict. Ого. Не ожидал. Другой подозрительный элемент связан с библиотекой pickle:
46859 0.178 0.000 0.347 0.000 pickle.py:1033(load_dict)
3 0.000 0.000 0.000 0.000 pickle.py:1093(load_global)
3 0.000 0.000 0.000 0.000 pickle.py:1128(find_class)
11825 0.032 0.000 0.088 0.000 pickle.py:1135(load_reduce)
225226 0.497 0.000 0.797 0.000 pickle.py:1156(load_get)
106928 0.180 0.000 0.258 0.000 pickle.py:1170(load_put)
175038 0.393 0.000 0.626 0.000 pickle.py:1199(load_setitem)
11825 0.091 0.000 0.271 0.000 pickle.py:1217(load_build)
58684 0.081 0.000 0.117 0.000 pickle.py:1256(load_mark)
1 0.000 0.000 0.000 0.000 pickle.py:1260(load_stop)
1 0.011 0.011 4.788 4.788 pickle.py:1386(loads)
1 0.000 0.000 0.000 0.000 pickle.py:83(__init__)
1 0.000 0.000 0.000 0.000 pickle.py:835(__init__)
1 1.183 1.183 4.777 4.777 pickle.py:851(load)
58684 0.094 0.000 0.132 0.000 pickle.py:876(marker)
88873 0.230 0.000 0.348 0.000 pickle.py:917(load_int)
2 0.000 0.000 0.000 0.000 pickle.py:967(load_string)
36414 0.116 0.000 0.168 0.000 pickle.py:985(load_unicode)
11825 0.027 0.000 0.056 0.000 pickle.py:999(load_tuple)
Остальные 40% были потрачены на pickle, некоторую библиотеку для сериализации / десериализации объектов Python.
Я был немного ошеломлен на этом этапе; ни одной медленной точки не было связано с кодом базы данных, что означало, что это была чисто алгоритмическая проблема в Python. И почему-то это было медленно, когда было больше команд.
Эти тихие воды текут глубоко
Я решил, что первым делом займусь AttrDict.deepcopy, так как это было медленное место. Это также оказалось неверным предположением, потому что я не полностью понимал, что делает этот код и почему, но мы скоро вернемся к этому.
AttrDict выглядело так (пушинка убрала):
class AttrDict(dict):
"""
Dict that allows attribute access
"""
def __getattr__(self, name):
try:
return self[name]
except KeyError:
return super(AttrDict, self).__getattribute__(name)
def __setattr__(self, name, value):
self[name] = value
@classmethod
def deepcopy(cls, other):
if isinstance(other, basestring):
return other
elif isinstance(other, collections.Sequence):
assert isinstance(other, list)
return [AttrDict.deepcopy(item) for item in other]
elif isinstance(other, collections.Mapping):
assert isinstance(other, (AttrDict, dict))
return AttrDict(
(key, AttrDict.deepcopy(value))
for key, value in other.iteritems()
)
else:
return other
Так что AttrDict был в основном питоном dict с некоторым чутьем. Одним из достоинств была возможность доступа к элементам словаря с точечным синтаксисом, аналогичным javascript. Поэтому вместо some_attrdict['blah'] вы можете написать some_attrdict.blah. Этот код, вероятно, был где-то скопирован из переполнения стека, поскольку тот, кто его написал, не был первым, кто пожелал этой функциональности.
Вторая часть была deepcopy, оскорбительный код. На первый взгляд, требуется какой-то объект с именем other, и он рекурсивно создает его глубокую копию. Один интересный побочный эффект от этого заключался в том, что dict объекта будут превращены в AttrDict.
Другая интересная часть заключалась в том, что этот код использовал isinstance и оскорбительно. isinstance является дорогостоящим, потому что ему необходимо проверить иерархию наследования объекта, чтобы узнать, относится ли предоставленный объект - или некоторый дочерний класс - к предоставленному типу.
Также было несколько бессмысленных утверждений (сноска: наши утверждения выполняются в производственной среде. Ни одно из них не сработало, пока у меня была история журнала, поэтому они бесполезны) и бессмысленная структура if / else.
Так что выигрывай. Я могу переписать это, чтобы улучшить производительность, и это может помочь. Я изменился на:
@classmethod
def deepcopy(cls, other):
other_type = type(other)
if other_type in [dict, AttrDict]:
return AttrDict(
(key, AttrDict.deepcopy(value))
for key, value in list(other.items())
)
elif other_type is list:
return [AttrDict.deepcopy(item) for item in other]
else:
return other
… Не было необходимости проверять строковые типы, потому что они следовали по тому же пути, что и путь else: return other.
И вместо того, чтобы использовать isinstance каждый раз, используйте type один раз и проверяйте наличие определенных типов, которые, как я знал, поддерживаются. Это изменение было сопряжено с риском: это означало, что я больше не мог должным образом обрабатывать унаследованные объекты, как это делал ранее код. Чтобы смягчить это, я подтвердил, что этот обновленный код дает те же результаты в производственной среде.
После настройки prod для повторного запуска этого кода, чтобы я мог получить чистый профиль, основная часть AttrDict теперь занимала примерно две секунды. Неплохо, но приемлемо. Учитывая, что это меньше, чем другой оскорбительный фрагмент кода, я решил перейти к pickle.
Это должно быть рассол
Сначала я нашел, где мы использовали рассол. Был фрагмент кода, который извлекал функции учетной записи и кэшировал их, чтобы избежать повторной генерации:
def _all_features(account_id):
cache_key = _get_cache_key(account_id)
account_features = cache.get(cache_key)
if account_features:
try:
account_features = AttrDict(pickle.loads(zlib.decompress(account_features)))
except (...):
...
else:
return account_features
account_features = _split_features(_query_features(account_id))
cache.set(cache_key, zlib.compress(pickle.dumps(account_features)))
return account_features
Базовый обзор:
- Попробуйте извлечь функции из кеша.
- Если они существуют, распакуйте с помощью zlib, увлажните их рассолом и верните их.
- Если их нет, сгенерируйте их заново, обезвожьте с помощью pickle, сожмите с помощью zlib, сохраните в кеш и верните их.
После прочтения этого кода у меня было больше вопросов, чем ответов. Несколько мыслей / идей, которые приходили мне в голову:
- Вся причина, по которой мы использовали
pickle, определенно была из-заAttrDict. В какой-то момент кто-то решил, что нам нужно кэшировать этот материал. Но поскольку мы использовалиAttrDict, нам нужен был способ сериализации / десериализации объектов Python. - Использование zlib было странным. Посоветовавшись с коллегой, я узнал , что это произошло из-за того, что объект кэша имел ограничение в 1 МБ, а для учетных записей со многими командами мы превысили этот предел. Поэтому сжатие zlib было добавлено в качестве временной меры.
Вторая часть меня просто поразила; что, черт возьми, сохранял рассол, который натолкнулся на ограничение в 1 МБ? Кроме того, если все, что мы в конечном итоге сохраняли, это объекты dict / list, какое это имело значение, если мы использовали pickle? Мы можем сериализовать с json.
Я быстро взломал терминал, чтобы посмотреть, как будет сравниваться JSON:
In [35]: json_compressed = zlib.compress(json.dumps(account_features)) In [36]: pickle_compressed = zlib.compress(pickle.dumps(account_features)) In [37]: len(pickle_compressed) Out[37]: 642794 In [38]: len(json_compressed) Out[38]: 50585
… Боже милостивый. Сжатая полезная нагрузка с pickle в двенадцать раз больше. Но становится еще хуже:
In [41]: start = time.clock()
...: json_compressed = zlib.compress(json.dumps(account_features))
...: print time.clock() - start
...:
0.035571
In [42]: start = time.clock()
...: pickle_compressed = zlib.compress(pickle.dumps(account_features))
...: print time.clock() - start
...:
1.201328
… Работа с полезной нагрузкой JSON была исключительно быстрее - примерно в 35 раз для гидратации / обезвоживания полезной нагрузки. Одна только замена с pickle на json решила бы эту проблему.
Но одна серьезная загвоздка: при использовании AttrDict, сбросе в JSON, сжатии, распаковке и последующей загрузке с помощью JSON мы полностью теряем AttrDict - он возвращается как dict, а не AttrDict. И об обновлении огромных фрагментов нашей кодовой базы, которая использовала AttrDict, не могло быть и речи. Я бы никогда не прошел через QA в разумные сроки и не смог бы быть уверен в таком масштабном изменении.
И тут я вспомнил deepcopy. Я мог сделать это:
account_features = AttrDict.deepcopy(json.loads(zlib.decompress(account_features)))
… При регидратации, и это автоматически преобразует dict в AttrDict. Задача решена. И добавление одной дополнительной deepcopy операции не имело значения, даже несмотря на то, что это был предыдущий код нарушения. Мы уже выполняли сотни из них (см. Профиль выше), и этот кодовый путь был вызван только один раз.
Изменить: было указано, что вместо использования deepcopy было бы предпочтительнее использовать функцию json.load object_hook:
json.loads( zlib.decompress(account_features), object_hook=AttrDict)
… Это было примерно в 5 раз лучше, чем стратегия deepcopy. Спасибо!
Кроме нерешенного: /
На тот момент это было молниеносно для аккаунтов с небольшим количеством команд и 2–3 секунды для аккаунтов с 500 командами. Итак, я создал учетную запись с тысячей команд, чтобы оценить производительность.
Я вернулся через десять секунд на team-*.
Именно в этот момент я собрал все воедино; Моя проблема с этой конечной точкой заключалась не в плохом коде Python, хотя он и был одним из факторов. Это было то, что у меня была большая проблема. Если 500 заняло у меня 2,5 секунды, а 1000 - 10 секунд, это почти идеально соответствовало O(n^2). Основная алгоритмическая проблема все еще существовала, и она росла с добавлением команд.
К сожалению, здесь мне пришлось признать еще одну разочаровывающую реальность инженерной мысли. Мои улучшения уже были достаточно хороши, чтобы пройти эту работу через QA. У нас не было аккаунтов с более чем 500 командами. Я попросил наших специалистов по контролю качества создать новый дефект для 1000 команд, чтобы отслеживать дальнейшее развитие проблемы, и закрыл сообщение об ошибке.
Как бы мне ни хотелось разобраться и исправить, идеальное - враг достаточно хорошего. Прощайте Амиго.
История Git, несколько выводов, несколько случайных размышлений
Я, однако, покопался в истории git, чтобы понять, что, черт возьми, заключалось в сделке с AttrDict и pickle:
- Он был написан за пять лет до этого младшим разработчиком. Целью определенно было удобство - возможность делать
some_attr_dict.blahвместоsome_attr_dict['blah']. Плохая логика if / else и бессмысленные утверждения также имели больше смысла для младшего разработчика. В то время, однако, кодовая база была небольшой, и, вероятно, это не имело большого значения дляAttrDict - Несколько дней спустя более старший разработчик добавил возможность
pickleэтого объекта, чтобы мы могли начать его кэширование. Кеширование было добавлено для решения проблем с производительностью. Неясно, что это за проблемы с производительностью; не было четкой истории того, почему это было сделано. - В какой-то момент, когда кеш взорвался по мере того, как мы получали аккаунты во все большем количестве команд, какой-то другой разработчик добавил
zlibсжатие в кеширование. Я не могу винить это решение; это было срочное исправление, когда в производстве начался взрывной рост.
Мои выводы из всего этого:
- Я думаю, что следует избегать добавления оболочки к базовым типам Python, таким как
dictиlist. Это решение имело серьезные последствия в будущем, поскольку казалось не более чем синтаксическим сахаром. - К сожалению, старший разработчик не подтолкнул младшего разработчика к простоте ради простоты. Хотя это вызывает:
- К сожалению, на момент написания
AttrDictнашего процесса проверки кода не существовало. Исходный код - это то, что я просил бы переписать или оправдать. - К сожалению, проблемы с производительностью, которые гарантировали
pickle, не были четко указаны в истории git, или какой-либо связанный дефект в системе отслеживания проблем. Это дает мне понять, почему наличие четких руководящих принципов в отношении сообщений о фиксации так жизненно важно для больших, долгосрочных баз кода. - Это интересное исследование долгосрочных последствий выбора программирования. Мне ясно, что это одно из тех мест, где «умный» код имел серьезные последствия, и бизнесу было бы лучше без него.
- Мне, как старшему разработчику, ясно, что важно наставлять младших разработчиков в направлении самого простого, что может сработать.
И еще несколько случайных заключительных размышлений:
- Оказывается, есть куча выражений СНУТ, которые ведут довольно запутанные дискуссии о
isinstance; Бьярн Страуструп даже возражал против включения такой функции в C ++, что выглядит просто потрясающе, поскольку этот язык, похоже, не находит намека на то, что не следует включать. pepруководящие принципы неявно подталкивают людей кisinstance, и это, пожалуй, не очень хорошо.isinstanceне особенно эффективен, и это абсолютно не то же самое, чтоtype. Копаясь в истории git, я обнаружил запрос на слияние, в котором старший разработчик прошел через нашу кодовую базу, бездумно заменяяtypeнаisinstance, потому чтоpepтак сказал.isinstance,typeи обработка строк отстой из-за проблем преобразования python 2 - ›python 3. Некоторое время я ломал голову надdeepcopyобработкой строк, пока не понял, что мне это вообще не нужно.
