Как понять хорош ли код?

Question

[Evgeniy]

Доброго времени суток. Создал недавно плагин для WordPress/WooCommerce. Его суть достать товары из базы данных MySQL и изменить их цену исходя из того какие данные вписаны в входящий CSV. Надеюсь понятно изложил. Код не приложил так как он довольно объемный, не думаю что кто-либо его серьёзно рассмотрит.
Суть проблемы в том, что 470 товаров обрабатываются за 30 секунд примерно. Получается 15 товаров в секунду. Насколько это быстро? Может быть это слишком медленно...
Не ругайте строго, я не знаю как проверить код и его оценить.

Comments

[Froggyweb]

Получается 3 товара в секунду

это сущий ад....
Хотя WP..... но нет.
таймаут сервера для исполнения скрипта по умолчанию 60 сек

[Evgeniy]

Froggyweb, был бы вам очень благодарен если конкретизируете. Скрипт работает быстро или медленно?

[Ипатьев]

Медленно - это не то слово
Нормальный код должен обрабатывать 10000-100000 товаров в секунду.

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

[alexalexes]

Приведите упрощенный пример, как вы производите операции, желательно с таймингом, чтобы понять, где у вас затык в скорости.
Например, можно составить такой лог:

0 мс: начало скрипта
12 мс: начало чтения файла
24 мс: конец чтения файла
30 мс: запрос единицы товара
34 мс: обновление цены единицы товара
35 мс: запрос единицы товара
36 мс: обновление цены единицы товара
 ....
12345 мс: конец скрипта.

[Evgeniy]

Роман Юрьевич Ипатьев, извините, я виноват. Не 100 товаров в 30 секунд а 470 товаров.

[Evgeniy]

alexalexes, извините, я виноват. Не 100 товаров в 30 секунд а 470 товаров

[Daria Motorina]

100 товаров за 30 секунд это ужасно медленно. Причина во многом может быть в кривых запросах к базе и отсутствии ее оптимизации (я знаю что в WP сильно много не поменяешь, но думаю что можно добавить индексы).
Код можно выложить на github/gitlab/bitbucket и приложить ссылку на репозиторий к вопросу и добавить тег "code review"

[Evgeniy]

Daria Motorina, прошу прощения ввел в заблуждение не 100 товаров а 470

[Evgeniy]

alexalexes, идеально, я даже об этом не подумал. Сразу вопрос задал. Большое вам спасибо.

[Daria Motorina]

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

[alexalexes]

Edgark, не бывает плохих скриптов, бывают бутылочные горлышки в стратегии обработки или взаимодействия, которые нужно понять как обходить. Для начала нужно сделать анализ скрипта, какие его части в каком интервале времени работают, и выделить проблему в тех частях, где тратится много времени.

[Evgeniy]

alexalexes, очень вам благодарен. В принципе вы дали мне то, за чем я и пришел на форум. Пожалуйста напишите тоже самое в ответах(про логи). Я отмечу ваш ответ решением)

[Evgeniy]

Daria Motorina, Дарья, слово "оптимизация" открыло мне глаза. Сейчас начал читать на тему оптимизация PHP кода и нашел много нового для себя. Очень вам благодарен.

[alexalexes]

Edgark, довольно очевидная вещь, если что-то непонятное творится в вашем коде, то выведите куда-нибудь что-нибудь, чтобы понять, что в черном ящике.

[Stalker_RED]

Edgark, Загугли ещё "php profiler"

[Evgeniy]

Stalker_RED, спасибо, уже загуглил и читаю.

[Evgeniy]

Всем большое спасибо за помощь)

[Evgeniy]

Daria Motorina, Дарья, я посмотрел как можно улучшить код и улучшил, но меня все равно не покидает ощущение, что есть ошибки, потому что скорость существенно не увеличилась. Если я пришлю вам ссылку на код, вы смогли бы просмотреть и оценить мой код?
Буду вам очень благодарен, если это возможно.
Написал с помощью ООП и комментарии есть у каждого метода.

[Stalker_RED]

Edgark, выложите код на гитхаб.

Скорее всего вы перебираете товары в цикле, по одному за раз, в то время как самым эффективным было бы обновить все цены одним SQL-запросом.

[Evgeniy]

Stalker_RED, вы верно заметили. Все так, но SQL-запрос я не мог сделать так как вариативные товары скрывают свои данные в сериализованом массиве. А от туда через SQL я достать их не смог. Возможно я усложняю...
https://github.com/Edgardo-developer/Plugin/blob/m...

[Evgeniy]

Stalker_RED, посмотрите пожалуйста и не скрывайте критики. Я готов её принять)

[P747]

Я бы на вашем месте запускал нужный код для обработки товара через очереди, например RabbitMQ

[Evgeniy]

P747, я еще очереди не проходил к сожалению)
Как я понял из php.net очередь доводит с начала до конца 1 товар(в данном случае) а потом начинает запускать 2-ой и так далее. Очередь мне напоминает генератор, но в более крупном масштабе. Генератор работает в пределах своего кода, и сразу как получает ответ передает его дальше не соединяя все ответы. Очередь же работает в довольно большом фрагменте кода. Интересная методика, но к сожалению, у меня нет опыта в ней, и я еще не дошел до уровня очередей. Но за совет очень благодарен)
Вы могли бы сказать есть ли ошибки в этом классе? Может следует использовать другую функцию где-то? Может я не логично использую ООП? Был бы вам очень благодарен за любую критику.

[P747]

Edgark, Вкратце очереди нужны для распараллеливания выполняемых процессов. Например, допустим у вас есть сайт для выдачи кредитов, тоесть после того как пользователь подал заявку на кредит(допустим заполнил свои данный и нажал на кнопку "Отправить заявку"). Далее вам нужно собрать все данные пользователя и принять решение, выдать кредит или нет, допустим на принятие решения уходит 10 минут. Вы можете это сделать несколькими способами после того как пользователь нажмет на кнопку "Отправить заявку": последовательно (тоесть пользователь будет ждать 10 минут пока обработается заявка, если не брать в расчет таймаут) или "отправить" заявку на обработку в очередь, а клиент в личном кабинете просто увидит что заявка в обработке.

[Evgeniy]

P747, спасибо что объяснили тему очередей. Пожалуйста напишите хотя бы 3 ошибки моего класса. Мне нужно знать в чем улучшаться, тем более как вы сказали они грубые. Буду вам очень благодарен)

[P747]

Edgark, Они не грубые, это я тупанул, кое с чем перепутал) Вообще советую писать код по PSR, сейчас это https://www.php-fig.org/psr/psr-12/

Это как СП в строительстве)
Можете вообщем рассказать что в техническом плане планируете в итоге сделать, какой функционал? Попробую подсказать по архитектуре вашего проекта

[Evgeniy]

P747, спасибо большое)
Суть плагина в том чтобы на входе получать CSV файл. В CSV файле расположены варианты товаров которые нужно изменить. Приведу одну строку в пример:
Категория Аргумент1 Аргумент2 Аргумент3 Цена
Итак, класс считывает все эти данные и отправляет уникальные значения в базу данных, получает все товары. После этого проходит по каждому товару и сверяет есть ли у какого-либо товара 3 атрибута из одной строки. Если есть меняется цена. В принципе вот и все. Это единственное что делает плагин. Просто, как подметили выше, плагин обрабатывает товары довольно медленно. 10-15 товаров в секунду. Я подменил preg_match() на strpos(), но проблема также не решилась. Немного оптимизировал код, удалил лишние переменные, операции и 1 метод. Но класс все равно не увеличил свою скорость.

[P747]

Edgark, Какие-нибудь идентификаторы есть по каждому товару в CSV?

[Evgeniy]

P747, не, CSV дает указания какие товары искать. По этим указаниям достаются товары и меняется цена. Идентификаторов изначально нет.

[P747]

Edgark, Ну если у вас постоянно идет сравнения на старые новые значения, с добавлением товаров новых только с уникальными значениями, то как вариант создать хэш индекс для таблицы товаров и по хэшам проверять из CSV какие товары нужно изменить, а какие новые добавить. Все таки как-то более подробно опишите алгоритм что хотите с атрибутами ценами товара сделать, а то выше несколько раз перечитал, несостыковки есть

[Evgeniy]

P747, хорошо, сейчас объясню более подробно. На сайте есть более 1000 товаров, они все вариативные. Допустим, 1000 товаров относятся к категории машины.
Я загружаю такое CSV:
Машины красная новая Toyota 43000
Это 1 строка из CSV. Класс достает все товары относящиеся к категории Машины. При этом достаются товары которые совпадают со следующим условием:

$category=="машины"||$color=="красный"||$status=="новый"||$model=="Toyota"

После этого полученные товары сверяются уже на полное совпадение со строкой. То есть вместо || уже стоит условие &&. Если вариация соответствует условию, то есть содержит нужные атрибуты, берется цена из строки, в которой атрибуты соответствуют атрибутам вариации и она применяется к данной вариации. Цена прописывается в метаданные.
Фактически плагин ищет товары на совпадение со строкой. Если есть совпадение то обновляет цену на нужную.

[P747]

Edgark, Ладно я завтра по подробнее на ваш код посмотрю, но скорее всего вам Sphinx нужно использовать

[Evgeniy]

P747, хорошо, спасибо

[P747]

Edgark, Привет) Два дня смотрел, так до конца и не понял, тут наверное подробно надо разбирать)

[Evgeniy]

P747, Привет. Я думал ты забыл уже про мой вопрос или нет времени на него. Спасибо что выделил время и посмотрел)

Answer 1

[alexalexes]

Для анализа производительности необходимо в вашем скрипте поставить контрольные точки, в которых будут логироваться текущее действие. Так можно понять, на что обратить внимание.
Например, можно составить такой лог:

0 мс: начало скрипта
12 мс: начало чтения файла
24 мс: конец чтения файла
30 мс: запрос единицы товара
34 мс: обновление цены единицы товара
35 мс: запрос единицы товара
36 мс: обновление цены единицы товара
 ....
12345 мс: конец скрипта.

PS: Логировать можно не только время, но и используемую память, и другие интересуемые параметры в момент фиксации точки.

Comments

[Stalker_RED]

Эх, тяжело тем людям, которые о профайлере не слышали. Приходится вручную всякие точки расставлять, потом анализировать ещё.

[Evgeniy]

Stalker_RED, как я понял профайлер хорош больше для проектов на чистом PHP без CMS. Не смог найти профайлеры для WordPress плагинов.

[Stalker_RED]

Edgark, профайлеру все равно что анализировать. Никакие особенные "профайлеры для wordpress" не нужны.
Самые популярные это xdebug, Blackfire, XHProf, Tideways,