Selector performance#72380

Merged

oatkiller merged 5 commits intoelastic:masterfrom

oatkiller:selector-performance

Jul 20, 2020

Contributor

oatkiller commented Jul 18, 2020 •

edited

Loading

Summary

Memoize various selectors
Improve performance of the selectors that calculate the aria-flowto attribute.
more tests.

No behaviour changes intended.

screenshot at `546079d`

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

oatkiller added 2 commits

July 18, 2020 14:55


          memoize many selectors

e309652


          fix aria flowto performance

9d4d985

oatkiller requested review from bkimmel and kqualters-elastic

July 18, 2020 20:22

oatkiller requested review from a team as code owners

July 18, 2020 20:22

oatkiller added release_note:skip v7.9.0 labels


          remove dead code

3356c2d

oatkiller commented

View reviewed changes

x-pack/plugins/security_solution/public/resolver/models/indexed_process_tree/index.ts

-              export function children(tree: IndexedProcessTree, process: ResolverEvent): ResolverEvent[] {
-                const id = uniquePidForProcess(process);
-                const currentProcessSiblings = tree.idToChildren.get(id);
+              export function children(tree: IndexedProcessTree, parentID: string | undefined): ResolverEvent[] {

Contributor Author

oatkiller Jul 18, 2020

This can return children for a node that isn't in the tree itself. For example, the root node of the tree may have a parent that isn't in the tree. Also, the root node may have no parent (undefined.)

x-pack/plugins/security_solution/public/resolver/models/indexed_process_tree/index.ts

                 if (rootNode !== null) {
-                  yield* baseLevelOrder(rootNode, children.bind(null, tree));
+                  yield* baseLevelOrder(rootNode, (parentNode: ResolverEvent): ResolverEvent[] =>
+                    children(tree, uniquePidForProcess(parentNode))

Contributor Author

oatkiller Jul 18, 2020

children now takes an id, since the parent may not be in-memory.

...ugins/security_solution/public/resolver/models/indexed_process_tree/isometric_taxi_layout.ts

                 for (const process of processesInReverseLevelOrder) {
-                  const children = model.children(indexedProcessTree, process);
+                  const children = model.children(indexedProcessTree, uniquePidForProcess(process));

Contributor Author

oatkiller Jul 18, 2020

children now takes an id, since the parent may not be in-memory.

...ugins/security_solution/public/resolver/models/indexed_process_tree/isometric_taxi_layout.ts

                   };
-                  const siblings = model.children(indexedProcessTree, parent);
+                  const siblings = model.children(indexedProcessTree, uniquePidForProcess(parent));

Contributor Author

oatkiller Jul 18, 2020

children now takes an id, since the parent may not be in-memory.

...ugins/security_solution/public/resolver/models/indexed_process_tree/isometric_taxi_layout.ts

                     };
-                    const siblings = model.children(tree, parent);
+                    const siblings = model.children(tree, uniquePidForProcess(parent));

Contributor Author

oatkiller Jul 18, 2020

children now takes an id, since the parent may not be in-memory.

x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts

                * Process events that will be graphed.
                */
-              export const graphableProcesses = createSelector(resolverTree, function (tree?) {
+              export const graphableProcesses = createSelector(resolverTreeResponse, function (tree?) {

Contributor Author

oatkiller Jul 18, 2020

renaming stuff

x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts

                * we're currently interested in.
                */
-              const resolverTree = (state: DataState): ResolverTree | undefined => {
+              const resolverTreeResponse = (state: DataState): ResolverTree | undefined => {

Contributor Author

oatkiller Jul 18, 2020

renaming stuff

x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts

    
              export function hasMoreChildren(state: DataState): boolean {

                const tree = resolverTree(state);

                return tree ? resolverTreeModel.hasMoreChildren(tree) : false;

                const resolverTree = resolverTreeResponse(state);

Contributor Author

oatkiller Jul 18, 2020

renaming stuff

x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts

    
              export function hasMoreAncestors(state: DataState): boolean {

                const tree = resolverTree(state);

                return tree ? resolverTreeModel.hasMoreAncestors(tree) : false;

                const resolverTree = resolverTreeResponse(state);

Contributor Author

oatkiller Jul 18, 2020

renaming stuff

x-pack/plugins/security_solution/public/resolver/store/data/selectors.ts

                       });
                     };
-                    const matchingEventsForCategory = defaultMemoize(unmemoizedMatchingEventsForCategory);

Contributor Author

oatkiller Jul 18, 2020

This would be garbage collected as there is no dominator


          remove console log

1fe12a1

oatkiller commented

View reviewed changes

x-pack/plugins/security_solution/public/resolver/store/selectors.ts

+               * Calls the `secondSelector` with the result of the `selector`. Use this when re-exporting a
+               * concern-specific selector. `selector` should return the concern-specific state.
+               */
+              function composeSelectors<OuterState, InnerState, ReturnValue>(

Contributor Author

oatkiller Jul 19, 2020

moved to bottom of file

oatkiller mentioned this pull request

[Resolver] Origin process #72382

Merged

11 tasks


          fix type issue

546079d

oatkiller force-pushed the selector-performance branch from bbce3b1 to 546079d Compare

July 19, 2020 06:08

Contributor

kibanamachine commented Jul 19, 2020

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 546079d

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.3MB	+496.0B	7.3MB

History

💚 Build #62814 succeeded 9d4d985

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

kqualters-elastic approved these changes

View reviewed changes

kqualters-elastic reviewed

View reviewed changes

x-pack/plugins/security_solution/public/resolver/store/camera/selectors.ts

-                    return (time) => {
+                    // memoizing this so the vector returned will be the same object reference if called with the same `time`.
+                    return defaultMemoize((time) => {

Contributor

kqualters-elastic Jul 20, 2020

any reason to not do this for all of the time based selectors?

Contributor Author

oatkiller Jul 20, 2020

I didn't do it for isAnimating because it returns a primitive and is trivial enough that there should be no perf gain to memoizing.
I didn't do it for visibleNodesAndEdgeLines because it just calls two functions which are each already memoized.
I think I did it everywhere else.

james-elastic approved these changes

View reviewed changes

michaelolo24 approved these changes

View reviewed changes

oatkiller merged commit 6cf796a into elastic:master

oatkiller deleted the selector-performance branch

July 20, 2020 13:38

oatkiller mentioned this pull request

[7.x] [Resolver] Selector performance (#72380) #72433

Merged

oatkiller pushed a commit to oatkiller/kibana that referenced this pull request


          [Resolver] Selector performance (elastic#72380)

93c833c

* Memoize various selectors
* Improve performance of the selectors that calculate the `aria-flowto` attribute.
* more tests.

oatkiller mentioned this pull request

[7.9] [Resolver] Selector performance (#72380) #72434

Merged

oatkiller pushed a commit to oatkiller/kibana that referenced this pull request


          [Resolver] Selector performance (elastic#72380)

4d2de2a

* Memoize various selectors
* Improve performance of the selectors that calculate the `aria-flowto` attribute.
* more tests.

oatkiller pushed a commit that referenced this pull request


          [Resolver] Selector performance (#72380) (#72434)

0f7aa41

* Memoize various selectors
* Improve performance of the selectors that calculate the `aria-flowto` attribute.
* more tests.

oatkiller pushed a commit that referenced this pull request


          [Resolver] Selector performance (#72380) (#72433)

ffc28c7

* Memoize various selectors
* Improve performance of the selectors that calculate the `aria-flowto` attribute.
* more tests.

gmmorris added a commit to gmmorris/kibana that referenced this pull request


          Merge branch 'master' into task-manager/batch-update-delete

d9a1b62

* master: (60 commits)
  [SIEM][Detection Engine][Lists] Adds list permissions (elastic#72335)
  [SIEM][Detection Engine][Lists] Adds conflict versioning and io-ts improvements to lists (elastic#72337)
  [Resolver] no longer pass related event stats to process node component (elastic#72435)
  Revert "skip flaky suite (elastic#72146)"
  [Security Solution] Cleanup endpoint telemetry (elastic#71950)
  Unskip dashboard embeddable rendering tests (elastic#71824)
  [ENDPOINT] Added unerolling status for host. (elastic#72303)
  [Alerting][Connectors] Increase the size of the logos (elastic#72419)
  [SECURITY] [Timeline] Raw events not displayed (elastic#72387)
  [ML] Fixes display of regression stop stats if one is NaN (elastic#72412)
  [Ingest Pipelines] Processor Editor Move Tooltip (elastic#72239)
  Fix match phrase and not match phrase comparators (elastic#71850)
  [Plugin Generator] Generate tsconfig and useDefaultBehaviors (elastic#72040)
  [Security Solution][Timeline] Fix timeline styling and createFrom beh… (elastic#72152)
  [Resolver] Selector performance (elastic#72380)
  [Ingest Manager] Set `_meta` in the index.mappings (elastic#72026)
  [Ingest Manager] Do not bumb config revision during config creation (elastic#72270)
  [ML] Adding missing index pattern name to new job wizards (elastic#72400)
  [ML] improve annotation flyout performance (elastic#72299)
  [APM] Testing error rate API and restructuring folders (elastic#72257)
  ...

gmmorris added a commit to gmmorris/kibana that referenced this pull request


          Merge branch 'master' into alerting/consumer-based-rbac

7febd47

* master: (26 commits)
  [SIEM][Detection Engine][Lists] Adds list permissions (elastic#72335)
  [SIEM][Detection Engine][Lists] Adds conflict versioning and io-ts improvements to lists (elastic#72337)
  [Resolver] no longer pass related event stats to process node component (elastic#72435)
  Revert "skip flaky suite (elastic#72146)"
  [Security Solution] Cleanup endpoint telemetry (elastic#71950)
  Unskip dashboard embeddable rendering tests (elastic#71824)
  [ENDPOINT] Added unerolling status for host. (elastic#72303)
  [Alerting][Connectors] Increase the size of the logos (elastic#72419)
  [SECURITY] [Timeline] Raw events not displayed (elastic#72387)
  [ML] Fixes display of regression stop stats if one is NaN (elastic#72412)
  [Ingest Pipelines] Processor Editor Move Tooltip (elastic#72239)
  Fix match phrase and not match phrase comparators (elastic#71850)
  [Plugin Generator] Generate tsconfig and useDefaultBehaviors (elastic#72040)
  [Security Solution][Timeline] Fix timeline styling and createFrom beh… (elastic#72152)
  [Resolver] Selector performance (elastic#72380)
  [Ingest Manager] Set `_meta` in the index.mappings (elastic#72026)
  [Ingest Manager] Do not bumb config revision during config creation (elastic#72270)
  [ML] Adding missing index pattern name to new job wizards (elastic#72400)
  [ML] improve annotation flyout performance (elastic#72299)
  [APM] Testing error rate API and restructuring folders (elastic#72257)
  ...

gmmorris added a commit to gmmorris/kibana that referenced this pull request


          Merge branch 'alerting/consumer-based-rbac' into alerting/management-…

c44b626

…feature-privileges

* alerting/consumer-based-rbac: (45 commits)
  fixed alerts test
  [SIEM][Detection Engine][Lists] Adds list permissions (elastic#72335)
  [SIEM][Detection Engine][Lists] Adds conflict versioning and io-ts improvements to lists (elastic#72337)
  [Resolver] no longer pass related event stats to process node component (elastic#72435)
  Revert "skip flaky suite (elastic#72146)"
  [Security Solution] Cleanup endpoint telemetry (elastic#71950)
  Unskip dashboard embeddable rendering tests (elastic#71824)
  [ENDPOINT] Added unerolling status for host. (elastic#72303)
  [Alerting][Connectors] Increase the size of the logos (elastic#72419)
  [SECURITY] [Timeline] Raw events not displayed (elastic#72387)
  [ML] Fixes display of regression stop stats if one is NaN (elastic#72412)
  [Ingest Pipelines] Processor Editor Move Tooltip (elastic#72239)
  Fix match phrase and not match phrase comparators (elastic#71850)
  [Plugin Generator] Generate tsconfig and useDefaultBehaviors (elastic#72040)
  [Security Solution][Timeline] Fix timeline styling and createFrom beh… (elastic#72152)
  allow user to disable alert even if they dont have privileges to the underlying action
  [Resolver] Selector performance (elastic#72380)
  [Ingest Manager] Set `_meta` in the index.mappings (elastic#72026)
  [Ingest Manager] Do not bumb config revision during config creation (elastic#72270)
  [ML] Adding missing index pattern name to new job wizards (elastic#72400)
  ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip v7.9.0