New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Selector performance #72380

Merged

oatkiller merged 5 commits into elastic:master from oatkiller:selector-performance

Jul 20, 2020

Contributor

oatkiller commented Jul 18, 2020 •

edited

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

@@ @@ -133,6 +129,8 @@ export function root(tree: IndexedProcessTree) { @@
               export function* levelOrder(tree: IndexedProcessTree) {
                 const rootNode = root(tree);
                 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

@@ @@ -226,7 +229,7 @@ function processEdgeLineSegments( @@
                     metadata: edgeLineMetadata,
                   };
-                  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

@@ @@ -420,7 +423,7 @@ function* levelOrderWithWidths( @@
                       parentWidth,
                     };
-                    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

@@ @@ -62,7 +62,7 @@ export function hasError(state: DataState): boolean { @@
                * The last ResolverTree we received, if any. It may be stale (it might not be for the same databaseDocumentID that
                * 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

                * `true` if there were more children than we got in the last request.
                */
               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

               }
               /**
                * `true` if there were more ancestors than we got in the last request.
                */
               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

@@ @@ -248,7 +255,7 @@ export const relatedEventInfoByEntityId: ( @@
                       });
                     };
-                    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

                       scalingConstants.maximum
                     );
-                    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 added 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 added 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 added 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 added 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