Skip to content
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.

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

[SIEM] Upgrades react-redux and utilize React.memo for performance gains #43029

Merged
merged 4 commits into from
Aug 10, 2019

Conversation

FrankHassanabad
Copy link
Contributor

@FrankHassanabad FrankHassanabad commented Aug 9, 2019

Summary

  • Upgrades react-redux so we can use React.memo with hooks and connected components.
  • Adds displayName to all the components that are React.memo as well as all components that are pure components.

Using React Profiler you can see that when doing something as innocuous as dragging the divider bar of the Timeline you will get updates of the different tables:

Screen Shot 2019-08-08 at 1 25 22 PM

This is a result of redux making changes based on the location of the timeline. As this occurs, the UrlState begins updating its self and all children which is not what we want.

You will see differences now in the snapshots such as this which adds naming to it:
Screen Shot 2019-08-09 at 2 34 06 PM

For the rest of us profiling we will have friendly names in our profiling tools to be able to trace what is going on very quickly

Before where a name didn't exist and it just showed Memo or pure:
Screen Shot 2019-08-09 at 11 55 08 AM

Now that a name exists you can trace to the code quickly to the component name:
Screen Shot 2019-08-09 at 3 08 42 PM

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [ ] This was checked for cross-browser compatibility, including a check against IE11

- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support

- [ ] Documentation was added for features that require explanation or tutorials

- [ ] Unit or functional tests were updated or added to match the most common scenarios

- [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately

- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@FrankHassanabad FrankHassanabad changed the title [SIEM] Upgrade react-redux and utilize React.memo for performance gains [SIEM] Upgrades react-redux and utilize React.memo for performance gains Aug 9, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem

@FrankHassanabad FrankHassanabad self-assigned this Aug 9, 2019
@@ -353,7 +353,7 @@ exports[`UrlStateContainer mounts and renders 1`] = `
}
}
>
<withRouter(Connect(UrlStateContainer))
<withRouter(Connect(Component))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I just read through it. I don't think it's worth doing the workaround to get the name back, if you're ok with that as sooner or later they will fix the bug.

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticeably better! Checked it out in the 3 main browsers, ran tests. Great pairing sesh yall! 🤠 LGTM once CI passes 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@@ -36,6 +37,6 @@ export function manageQuery<T>(WrappedComponent: React.ComponentClass<T> | React
return <WrappedComponent {...otherProps} />;
}
}

ManageQuery.displayName = `ManageQuery (${WrappedComponent.displayName || 'Unknown'})`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works really well in the React profiler 👍

Copy link
Contributor

@andrew-goldstein andrew-goldstein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the perf improvement, and for improving the developer experience when using the profiler! It's waaaaay easier now to read the flame graph when, for example, connected components are rendered.

I verified the UrlStateContainer performance improvement by running the React profiler against master first, and then comparing this PR to the baseline.

LGTM 🚀

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@FrankHassanabad FrankHassanabad merged commit fe3422c into elastic:master Aug 10, 2019
@FrankHassanabad FrankHassanabad deleted the use-memo-url-state branch August 10, 2019 01:46
FrankHassanabad added a commit to FrankHassanabad/kibana that referenced this pull request Aug 10, 2019
…ins (elastic#43029)

## Summary

* Upgrades react-redux so we can use React.memo with hooks and connected components.
* Adds displayName to all the components that are React.memo as well as all components that are `pure` components.

Using React Profiler you can see that when doing something as innocuous as dragging the divider bar of the Timeline you will get updates of the different tables:

<img width="1547" alt="Screen Shot 2019-08-08 at 1 25 22 PM" src="https://user-images.githubusercontent.com/1151048/62789872-19efad80-ba87-11e9-92b5-eb9666c7c07a.png">

This is a result of redux making changes based on the location of the timeline. As this occurs, the UrlState begins updating its self and all children which is not what we want.

You will see differences now in the snapshots such as this which adds naming to it:
<img width="1582" alt="Screen Shot 2019-08-09 at 2 34 06 PM" src="https://user-images.githubusercontent.com/1151048/62808504-7a95df00-bab5-11e9-89c1-910b3908c705.png">

For the rest of us profiling we will have friendly names in our profiling tools to be able to trace what is going on very quickly

Before where a name didn't exist and it just showed `Memo` or `pure`:
<img width="221" alt="Screen Shot 2019-08-09 at 11 55 08 AM" src="https://user-images.githubusercontent.com/1151048/62809192-79fe4800-bab7-11e9-993b-2e7abadbb691.png">

Now that a name exists you can trace to the code quickly to the component name:
<img width="1570" alt="Screen Shot 2019-08-09 at 3 08 42 PM" src="https://user-images.githubusercontent.com/1151048/62809235-9a2e0700-bab7-11e9-8711-bae7d263d487.png">


### Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~

~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~

~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~

~~- [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios~~

~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~

### For maintainers

~~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~

~~- [ ] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~
FrankHassanabad added a commit that referenced this pull request Aug 10, 2019
…ins (#43029) (#43070)

## Summary

* Upgrades react-redux so we can use React.memo with hooks and connected components.
* Adds displayName to all the components that are React.memo as well as all components that are `pure` components.

Using React Profiler you can see that when doing something as innocuous as dragging the divider bar of the Timeline you will get updates of the different tables:

<img width="1547" alt="Screen Shot 2019-08-08 at 1 25 22 PM" src="https://user-images.githubusercontent.com/1151048/62789872-19efad80-ba87-11e9-92b5-eb9666c7c07a.png">

This is a result of redux making changes based on the location of the timeline. As this occurs, the UrlState begins updating its self and all children which is not what we want.

You will see differences now in the snapshots such as this which adds naming to it:
<img width="1582" alt="Screen Shot 2019-08-09 at 2 34 06 PM" src="https://user-images.githubusercontent.com/1151048/62808504-7a95df00-bab5-11e9-89c1-910b3908c705.png">

For the rest of us profiling we will have friendly names in our profiling tools to be able to trace what is going on very quickly

Before where a name didn't exist and it just showed `Memo` or `pure`:
<img width="221" alt="Screen Shot 2019-08-09 at 11 55 08 AM" src="https://user-images.githubusercontent.com/1151048/62809192-79fe4800-bab7-11e9-993b-2e7abadbb691.png">

Now that a name exists you can trace to the code quickly to the component name:
<img width="1570" alt="Screen Shot 2019-08-09 at 3 08 42 PM" src="https://user-images.githubusercontent.com/1151048/62809235-9a2e0700-bab7-11e9-8711-bae7d263d487.png">


### Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~- [ ] This was checked for cross-browser compatibility, [including a check against IE11](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility)~~

~~- [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/master/packages/kbn-i18n/README.md)~~

~~- [ ] [Documentation](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#writing-documentation) was added for features that require explanation or tutorials~~

~~- [ ] [Unit or functional tests](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#cross-browser-compatibility) were updated or added to match the most common scenarios~~

~~- [ ] This was checked for [keyboard-only and screenreader accessibility](https://developer.mozilla.org/en-US/docs/Learn/Tools_and_testing/Cross_browser_testing/Accessibility#Accessibility_testing_checklist)~~

### For maintainers

~~- [ ] This was checked for breaking API changes and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~

~~- [ ] This includes a feature addition or change that requires a release note and was [labeled appropriately](https://github.com/elastic/kibana/blob/master/CONTRIBUTING.md#release-notes-process)~~
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 12, 2019
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (22 commits)
  [Code]: downgrade the log level of error message from subprocess (elastic#42925)
  [Code] Cancel clone/update job in the middle if disk space over the watermark (elastic#42890)
  Add Kibana App specific URL to the help menu (elastic#34739) (elastic#42580)
  [Maps] refactor createShapeFilterWithMeta to support more than just polygons (elastic#43042)
  Skip flaky es_ui_shared/request tests.
  Pass uiSettings to all data plugin services (elastic#42159)
  [SIEM] Upgrades react-redux and utilize React.memo for performance gains  (elastic#43029)
  [skip-ci][Maps] add maki icon sheet to docs (elastic#43063)
  Adding "style-src 'unsafe-inline' 'self'" to default CSP rules (elastic#41305)
  Update dependency commander to v3 (elastic#43041)
  Update dependency @percy/agent to ^0.10.0 (elastic#40517)
  [Maps] only show top hits checkbox if index has date fields (elastic#43056)
  run chained_controls on Firefox to catch regression (elastic#43044)
  fixing issue with dashboard csv download (elastic#42964)
  Expose task manager as plugin instead of server argument (elastic#42966)
  Expose createRouter from HttpService, prepare handlers for context introduction (elastic#42686)
  [Code] disk watermark supports percentage and absolute modes (elastic#42987)
  [apps/dashboard] skip part of filtering tests on FF (elastic#43047)
  [ML] Kibana management jobs list (elastic#42570)
  [ML] Fix check for watcher being enabled (elastic#43025)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants