Optimize memoized selector comparators #1214

brusherru · 2018-05-16T17:22:46Z

Still tweaks IDE performance, that was reduced in the v0.20.0 by adding generic types and type deduction.

nkrkv · 2018-05-17T08:31:23Z

packages/xod-client/src/utils/memoizedSelectorsTools.js

+);
+
+// :: (Patch -> Patch -> Boolean) -> Maybe Patch -> Maybe Patch -> Boolean
+export const maybePatchEqualsBy = R.curry(


Hmm… looks like it’s a job for lift:

export const maybePatchEqualsBy = R.curry( (compFn, prevMaybePatch, nextMaybePatch) => R.liftN(3, patchListEqualsBy) (Maybe(compFn), prevMaybePatch, nextMaybePatch) .getOrElse(false) ) );

🤔

It's not for a Maybe [Patch]

See code in Ramtuary

nkrkv · 2018-05-17T08:32:05Z

packages/xod-client/src/utils/memoizedSelectorsTools.js

+const getSetOfPatchPaths = R.pipe(R.keys, x => new Set(x));
+
+// :: (Patch -> Patch -> Boolean) -> [Patch] -> [Patch] -> Boolean
+export const patchListEqualsBy = R.curry(


Looks quite general to be moved to xod-project

nkrkv · 2018-05-17T08:37:57Z

packages/xod-client/src/utils/memoizedSelectorsTools.js

+    const diffPatchPaths = new Set(
+      [...prevPatchPaths].filter(x => !nextPatchPaths.has(x))
+    );
+    if (diffPatchPaths.size > 0) return false;


😮 such a shame, JS has no set comparisons out of the box

nkrkv · 2018-05-17T08:40:35Z

packages/xod-client/src/utils/memoizedSelectorsTools.js

+import { explodeMaybe } from 'xod-func-tools';
+import * as XP from 'xod-project';
+
+const getSetOfPatchPaths = R.pipe(R.keys, x => new Set(x));


Quite general to be named keySet and moved to xod-func-tools/set. Many missing ops might belong there.

nkrkv · 2018-05-17T08:41:19Z

packages/xod-project/src/patch.js

+// =============================================================================
+
+// :: StrMap Node -> Set Node
+const getSetOfNodeIds = R.pipe(R.keys, a => new Set(a));


Yup. keySet again

nkrkv · 2018-05-17T08:42:43Z

packages/xod-project/src/patch.js

+
+// =============================================================================
+//
+// Functions that checks could something change in the Patch


...checks whether something could change...

nkrkv · 2018-05-17T08:53:04Z

packages/xod-project/src/patch.js

+ * could affect on generic pin deduction or marked as
+ * "deprecated", "utility" or "dead".
+ *
+ * True if have changes.


Pass it through Grammarly, please 😬

Checks if a Patch has some changes over the Nodes that
could affect on generic pin deduction, or the patch changed its
deprecated/utility/dead state
Returns true if the patch has such changes

nkrkv · 2018-05-17T09:01:55Z

packages/xod-project/src/patch.js

+ * True if have changes.
+ */
+export const haveAddedNodesOrChangedTypesOrBoundValues = def(
+  'haveAddedNodesOrChangedTypesOrBoundValues :: Patch -> Patch -> Boolean',


I think the name does not scale well. You use isColdOrForecastIsBadOrLeaveForALongTime instead of shouldDressCoat. The latter will work better as it more straightforward and does not depend on the exact such of conditions that may evolve.

~~So, in this case, something like hasChangesObservableOutside will be more meaningful IMO, if the function is indeed about it. Please, confirm.~~

A proper naming will immediately show a problem. You use haveAddedNodesOrChangedTypesOrBoundValues for both: project browser selector and deduced pin types selector. But they are interested in different set of criterias. Should be three functions 🤔

couldChangeProjectBrowserLook (move to xod-client)

couldAffectTypeDeductionInside

couldAffectTypeDeductionOutside

nkrkv · 2018-05-17T09:12:14Z

packages/xod-project/src/patch.js

+ *
+ * True if have changes.
+ */
+export const haveAddedOrChangedNodes = def(


Is it used?

nkrkv · 2018-05-18T15:18:34Z

packages/xod-func-tools/src/objects.js

+export const compareMapsBy = def(
+  'compareMapsBy :: (b -> StrMap a) -> b -> b -> Boolean',
+  (mapGetter, prev, next) => {
+    // If same nodes maps — nothing changed


The node term is unknown in xod-func-tools. Please, rewrite in a more general fashion.

nkrkv · 2018-05-18T15:18:40Z

packages/xod-func-tools/src/objects.js

+    const prevMapKeys = setOfKeys(prevMap);
+    const nextMapKeys = setOfKeys(nextMap);
+    const diffMapKeys = diffSet(prevMapKeys, nextMapKeys);
+    // If there is some nodes added/deleted


nkrkv · 2018-05-18T15:27:33Z

packages/xod-func-tools/src/objects.js

@@ -117,3 +118,21 @@ export const invertMap = def(
  'invertMap :: Map a b -> Map b a',
  R.compose(R.fromPairs, R.map(R.reverse), R.toPairs)
 );
+
+export const compareMapsBy = def(
+  'compareMapsBy :: (b -> StrMap a) -> b -> b -> Boolean',


Exported func. A doc comment is missing. The name is misleading a bit. Hard to choose a proper name, but something like mapsToObjectsWithSameKeys or mapsToSameKeyset is closer to the truth.

sameKeysetBy?

nkrkv · 2018-05-18T15:30:22Z

packages/xod-project/src/patch.js

+);
+
+export const couldTypeDeductionChange = def(
+  'couldTypeDeductionChange :: Patch -> Patch -> Boolean',


Type deduction is a process. It can’t change. Did you mean couldDeducedTypesChange?

nkrkv · 2018-05-18T15:37:56Z

packages/xod-client/src/projectBrowser/selectors.js

+  XP.patchListEqualsBy(R.complement(XP.couldValidityChange))
+);
+
+const projectChangesNotAffectProjectBrowserLook = R.either(


Quite hard to reason about functions which are inverted in their name. Subjective and arguable, but will it make the code simpler to understand if you inverse the meaning, i.e., make libChangesKeepBrowserLook and patchListChangesKeepBrowserLook.

nkrkv · 2018-05-18T16:12:18Z

packages/xod-project/src/patch.js

+  listNodes
+);
+
+export const didUtilityOrDeprecatedMarkersChange = def(


How about didUtilityOrDeprecatedOrExampleOrFeaturedMarkersChange?

Suggest didCategoryMarkersChange

nkrkv · 2018-05-18T16:14:16Z

packages/xod-client/src/project/selectors.js

 export const getDeducedPinTypes = createMemoizedSelector(
  [getProject, getCurrentPatch],
-  [R.equals, R.equals],
+  [
+    projectHasSamePatchList,


A dangerous presumption. Will work only if no external impact can affect used nodes exterior. Will stale if we have split view.

brusherru · 2018-05-21T12:28:40Z

@nkrkv @evgenykochetkov, check it out, please

evgenykochetkov · 2018-05-21T12:36:26Z

packages/xod-func-tools/src/monads.js

+  'maybeEqualsWith :: (a -> a -> Boolean) -> Maybe a -> Maybe a -> Boolean',
+  (compFn, prevMaybe, nextMaybe) => {
+    if (prevMaybe === nextMaybe) return true;
+    if (Maybe.isNothing(nextMaybe) || Maybe.isNothing(prevMaybe)) return false;


Should it return true if both are Nothing?

nkrkv · 2018-05-21T13:07:57Z

packages/xod-client/src/project/selectors.js

+        nextPatches
+      );
+    },
+  ],


Inlining the comparators makes getDeducedPinTypes harder to follow. Suggest extracting to a function

nkrkv · 2018-05-21T13:11:31Z

packages/xod-func-tools/src/monads.js

@@ -109,6 +109,32 @@ export const catMaybies = R.compose(
  R.filter(Maybe.isJust)
 );

+export const maybeEqualsWith = def(
+  'maybeEqualsWith :: (a -> a -> Boolean) -> Maybe a -> Maybe a -> Boolean',


It’s an R.liftN(2) followed by Maybe.getOrDefault(false).

That would return false for two Nothings, which we (probably) don't want

nkrkv

LGTM. Extract the function and merge.

…ssive recalculations

brusherru added the t:perf label May 16, 2018

brusherru self-assigned this May 16, 2018

brusherru requested a review from a team May 16, 2018 17:22

brusherru changed the base branch from master to 0.20.x May 16, 2018 17:22

nkrkv requested changes May 17, 2018

View reviewed changes

nkrkv added the wip label May 18, 2018

brusherru force-pushed the perf-type-deduction-2 branch 2 times, most recently from 664bcc5 to 4fffb89 Compare May 18, 2018 14:02

brusherru removed the wip label May 18, 2018

brusherru force-pushed the perf-type-deduction-2 branch from 37bc551 to f996f66 Compare May 18, 2018 15:29

nkrkv requested changes May 18, 2018

View reviewed changes

nkrkv added the wip label May 18, 2018

nkrkv changed the title ~~Tweak performance by optimisation of memoized selector comparators~~ Optimize memoized selector comparators May 21, 2018

brusherru force-pushed the perf-type-deduction-2 branch 2 times, most recently from 3d54b28 to cb92c80 Compare May 21, 2018 10:41

brusherru removed the wip label May 21, 2018

brusherru force-pushed the perf-type-deduction-2 branch from cb92c80 to 9801902 Compare May 21, 2018 12:28

evgenykochetkov reviewed May 21, 2018

View reviewed changes

evgenykochetkov approved these changes May 21, 2018

View reviewed changes

brusherru force-pushed the perf-type-deduction-2 branch from 9801902 to c843507 Compare May 21, 2018 13:10

nkrkv requested changes May 21, 2018

View reviewed changes

brusherru force-pushed the perf-type-deduction-2 branch from c843507 to 7d89d89 Compare May 21, 2018 13:32

nkrkv approved these changes May 21, 2018

View reviewed changes

perf(xod-project, xod-client): tweak memoized selectors to avoid exce…

9dc27ed

…ssive recalculations

brusherru force-pushed the perf-type-deduction-2 branch from 7d89d89 to 9dc27ed Compare May 21, 2018 14:42

brusherru merged commit 4e49ff3 into 0.20.x May 21, 2018

brusherru deleted the perf-type-deduction-2 branch May 21, 2018 15:07

brusherru mentioned this pull request May 21, 2018

Merge 0.20.x to master #1229

Merged

nkrkv mentioned this pull request May 22, 2018

Update changelog for upcoming 0.20.3 #1233

Merged

nkrkv added the hotfix Issue/PR for a patch release label May 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimize memoized selector comparators #1214

Optimize memoized selector comparators #1214

brusherru commented May 16, 2018

nkrkv May 17, 2018

brusherru May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 17, 2018

nkrkv May 18, 2018

nkrkv May 18, 2018

nkrkv May 18, 2018

brusherru May 21, 2018

nkrkv May 18, 2018

nkrkv May 18, 2018

nkrkv May 18, 2018

nkrkv May 18, 2018

brusherru commented May 21, 2018

evgenykochetkov May 21, 2018

nkrkv May 21, 2018

nkrkv May 21, 2018

evgenykochetkov May 21, 2018

nkrkv left a comment

Optimize memoized selector comparators #1214

Optimize memoized selector comparators #1214

Conversation

brusherru commented May 16, 2018

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

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

brusherru commented May 21, 2018

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

nkrkv left a comment

Choose a reason for hiding this comment