perf: remove immer from static report #501

KuznetsovRoman · 2023-09-11T12:20:42Z

Что сделано

Избавление от immer в статическом отчете. Вместо immer, самостоятельно храним объект с правками. Так, state остается неизменным, а правки пишутся в объект diff. Затем новый стейт формируется из старого, на который наслоен diff. К этому механизму прилагаются три хелпера: applyStateUpdate (создание нового state из старого и diff), ensureDiffProperty (создание вложенного объекта при необходимости, используется при передаче diff в другую функцию), getUpdatedProperty (достает проп из state, учитывая, что он может быть перезаписан в diff).

В среднем, на больших отчетах задержка от immer при действиях - от 400 (если у пользователя мощное устройство и задействован только один тяжелый редьюсер (tree / grouped tests)) до 1600 (если у пользователя более слабое устройство и задействовано сразу два тяжелых редюсера)

Чтобы оценить, насколько избавление от immer ускоряет отчет на своем устройстве:

Отчет 1: https://nda.ya.ru/t/Wt4Uu-Mb6f4HVK
Отчет 2: https://nda.ya.ru/t/e1iRtpfT6f4HpB

Смотреть нужно на:

Выбор браузеров
Режим отображения тестов (failed/all/...)
Группировки
expand all / collapse all

KuznetsovRoman · 2023-09-11T12:21:30Z

lib/static/modules/reducers/grouped-tests/by/meta.js


-export function groupMeta({group, groupKey, ...restArgs}) {
+export function groupMeta({group, groupKey, diff = group, ...restArgs}) {


Здесь и далее diff в функциях - место, куда нужно записывать изменения. По иерархии такие diff равны тому, чему устанавливаются по умолчанию (в данном случае, group)

Довольно непонятное API. Если ничего не передал в diff значит изменения будут происходить на group.

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

Не пробовал смотреть на immutableJS? Судя по performance графикам от immer - https://immerjs.github.io/immer/performance/ он довольно шустрый.

Если ничего не передал в diff значит изменения будут происходить на group

Да, так и задумывалось. Это опциональный аргумент "куда записывать изменения". По умолчанию, если не передать его, запишет в group, плюс, это напоминание, что туда нужно передавать

нужно думать о том, как его правильно сформировать

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

Не пробовал смотреть на ImmutableJS?

Пробовал, еще более неудобный

KuznetsovRoman · 2023-09-11T12:21:55Z

lib/static/modules/reducers/grouped-tests/by/meta.js

+                group: group.byKey[groupKey],
+                result,
+                value,
+                diff: diff.byKey[groupKey]


Так что дальше в функции они прокидываются так же, как и сами их read-only сущности из state

KuznetsovRoman · 2023-09-11T12:22:33Z

lib/static/modules/reducers/tree/index.js

        case actionNames.INIT_GUI_REPORT:
        case actionNames.INIT_STATIC_REPORT: {


В этом action, tree не является частью стейта, так что тут можно мутировать его напрямую, что гораздо проще по коду

KuznetsovRoman · 2023-09-11T12:25:10Z

lib/static/modules/reducers/tree/index.js

@@ -17,80 +17,65 @@ import {ViewMode} from '../../../../constants/view-modes';
 import {EXPAND_RETRIES} from '../../../../constants/expand-modes';


Тут я перенес только action из статического отчета, потому что в gui профит слабый, да и логика там становится намного сложнее, так что оно того не стоит

Я пытался расположить обработку action в том же порядке, но тем не менее, в секции с reduce он отрезолвился плохо. Там можно не смотреть, оставил как было.

Редьюсер распилен на две части:

Первая (статика, без immer.produce):

switch (action.type) { case actionNames... .... return applyStateUpdate(state, diff)

Вторая (gui, с immer.produce):

return produce(state, draft => { switch (action.type) { case actionNames... .... break; })`

DudaGod

Мне не нравится текущий подход. Он получился довольно сложным и вербозным. Нужно думать о правильном формировании diff-а и таскать его за собой в каждую утилитку и формировать ее значение в ней. Это приводит к тому, что код становится трудно поддерживаемым, так как логика по формированию диффа размазана.

Я бы попробовал заюзать immutableJS - https://redux.gitbook.io/docs/recipes/usingimmutablejs. Судя по данным графикам - https://immerjs.github.io/immer/performance/ он работает существенно быстрее immera. И его использование не должно привезти к существенном усложнению кода.

DudaGod · 2023-09-14T10:26:30Z

lib/static/modules/utils.js

+    return result;
+}
+
+function ensureDiffProperty(diff, path) {


А почему вместо этого нельзя просто юзать _.set из lodash. Он же тоже самое делает - _.set(diff, path, {})

Нет, это две разные функции.

_.set - устанавливает значение.

_.set({a: {foo: 4, bar: 9}}, 'a', {}) вернет {a: {}}

Эта функция устанавливает значение только при необходимости

DudaGod · 2023-09-14T10:42:27Z

lib/static/modules/utils.js

@@ -159,6 +159,40 @@ function preloadImage(url) {
    new Image().src = url;
 }

+function applyStateUpdate(state, diff) {


А почему эти методы по работе со стейтом не вынести в отдельных utils/state? А то сейчас же мешанина из этих утилит.

Можно и вынести

DudaGod · 2023-09-14T11:21:52Z

lib/static/modules/reducers/api-values.js


 export default (state, action) => {
    switch (action.type) {
        case actionNames.INIT_GUI_REPORT:
        case actionNames.INIT_STATIC_REPORT: {
            const {apiValues} = action.payload;

-            return applyChanges(state, apiValues);
+            return applyStateUpdate(state, {apiValues});


А почему просто не назвать updateState?

потому что state не обновляется. Он иммутабелен, и updateState по сути ничего обновлять не будет. Тут именно создается новый стейт из старого и диффа

DudaGod · 2023-09-14T11:38:53Z

lib/static/modules/reducers/grouped-tests/by/meta.js


-export function groupMeta({group, groupKey, ...restArgs}) {
+export function groupMeta({group, groupKey, diff = group, ...restArgs}) {


Довольно непонятное API. Если ничего не передал в diff значит изменения будут происходить на group.

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

Не пробовал смотреть на immutableJS? Судя по performance графикам от immer - https://immerjs.github.io/immer/performance/ он довольно шустрый.

DudaGod · 2023-09-14T11:41:51Z

lib/static/modules/reducers/tree/nodes/results.js

    });
 }

-export function changeResultState(tree, resultId, state) {
-    changeNodeState(tree.results.stateById, resultId, state);
+export function changeResultState(tree, resultId, state, diff = tree) {


Скоро функция от кол-ва аргументов лопнет. Обычно стараемся при наличии 4 аргументов упаковывать их в объекты

Упакую

KuznetsovRoman commented Sep 11, 2023

View reviewed changes

KuznetsovRoman mentioned this pull request Sep 11, 2023

perf: increase html-reporter performance #492

Closed

DudaGod reviewed Sep 14, 2023

View reviewed changes

KuznetsovRoman force-pushed the HERMIONE-1138.perf_remove_immer branch 3 times, most recently from 44c1b12 to 576919b Compare September 14, 2023 22:52

perf: remove immer from static report

43ba93f

KuznetsovRoman force-pushed the HERMIONE-1138.perf_remove_immer branch from 576919b to 43ba93f Compare September 14, 2023 23:26

KuznetsovRoman merged commit f61b99a into master Sep 14, 2023
5 checks passed

KuznetsovRoman deleted the HERMIONE-1138.perf_remove_immer branch September 14, 2023 23:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf: remove immer from static report #501

perf: remove immer from static report #501

KuznetsovRoman commented Sep 11, 2023

KuznetsovRoman Sep 11, 2023

DudaGod Sep 14, 2023

KuznetsovRoman Sep 14, 2023

KuznetsovRoman Sep 11, 2023

KuznetsovRoman Sep 11, 2023

KuznetsovRoman Sep 11, 2023

DudaGod left a comment

DudaGod Sep 14, 2023

KuznetsovRoman Sep 14, 2023

DudaGod Sep 14, 2023

KuznetsovRoman Sep 14, 2023

DudaGod Sep 14, 2023

KuznetsovRoman Sep 14, 2023

DudaGod Sep 14, 2023

DudaGod Sep 14, 2023

KuznetsovRoman Sep 14, 2023


		export function groupMeta({group, groupKey, ...restArgs}) {
		export function groupMeta({group, groupKey, diff = group, ...restArgs}) {

		case actionNames.INIT_GUI_REPORT:
		case actionNames.INIT_STATIC_REPORT: {

		@@ -17,80 +17,65 @@ import {ViewMode} from '../../../../constants/view-modes';
		import {EXPAND_RETRIES} from '../../../../constants/expand-modes';

perf: remove immer from static report #501

perf: remove immer from static report #501

Conversation

KuznetsovRoman commented Sep 11, 2023

Что сделано

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DudaGod left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment