Fix: Prevent some needless re-rendering in high-level components #31958

w33ble · 2019-02-25T21:06:07Z

In an effort to remove the workpad renaming lag in #25070, I started hunting down and fixing re-renders that happened with the name change.

I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown:

Workpad, WorkpadHeader, and FullscreenControl would always re-render because the hotkey handler function was always re-created
Workpad would re-render when the workpad's name changed
- We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes
- Page title updating moved to middleware so the component doesn't need that value
AssetManager would always re-render if the parent re-rendered because it always created a new state value in mapStateToProps
make Workpad and Toolbar pure, they take no props and this helps stop some re-rendering

elasticmachine · 2019-02-25T21:06:09Z

Pinging @elastic/kibana-canvas

elasticmachine · 2019-02-25T21:46:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-26T00:06:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-26T01:05:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-26T01:44:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

allow setting the keyhandler function on the component instance

flat state prevents re-renders, and not using the workpad name prevents re-renders. plus, side effects should happen in middleware

help prevent re-renders on function redefinition

don't mutate values in mapStateToProps, so the connect function retains pure functionality

neither take props, so this stops some re-rendering

elasticmachine · 2019-02-28T22:37:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

clintandrewhall · 2019-02-28T23:35:40Z

x-pack/plugins/canvas/public/components/fullscreen_control/fullscreen_control.js

@@ -9,6 +9,12 @@ import PropTypes from 'prop-types';
 import { Shortcuts } from 'react-shortcuts';

 export class FullscreenControl extends React.PureComponent {
+  keyHandler = action => {
+    if (action === 'FULLSCREEN' || (this.props.isFullscreen && action === 'FULLSCREEN_EXIT')) {


nit: can we make these consts?

~~I don't understand what you're asking for here.~~

Is this what you're looking for?

export class FullscreenControl extends React.PureComponent { keyHandler = action => { - if (action === 'FULLSCREEN' || (this.props.isFullscreen && action === 'FULLSCREEN_EXIT')) { + const enterFullscreen = action === 'FULLSCREEN'; + const exitFullscreen = this.props.isFullscreen && action === 'FULLSCREEN_EXIT'; + + if (enterFullscreen || exitFullscreen) { this.toggleFullscreen(); } };

clintandrewhall · 2019-02-28T23:36:38Z

x-pack/plugins/canvas/public/components/workpad/workpad.js

-          <Shortcuts name="EDITOR" handler={keyHandler} targetNodeSelector="body" global />
-        )}
+    const bufferStyle = {
+      height: isFullscreen ? height : height + 32,


nit: can we make 32 a const? It will help explain what it is and why it's necessary.

I'm not sure why that value is there, I didn't change that. I'll try figuring it out though.

cqliu1

Excellent refactors! I threw a bunch of console.logs in all the render functions effected, and there are significantly less unnecessary re-renders when making changes to the workpad name, size, etc. I only have one requested change.

cqliu1 · 2019-02-28T20:03:40Z

x-pack/plugins/canvas/public/components/workpad/workpad.js

@@ -63,84 +69,85 @@ export const Workpad = props => {
    }
  };

-  setDocTitle(workpad.name);
+  render() {
+    console.log('Workpad render');


Please remove ^

cqliu1 · 2019-02-28T20:06:32Z

x-pack/plugins/canvas/public/state/middleware/workpad_update.js


  next(action);

+  // This middleware updates the page title when the workpad name changes
+  if (getWorkpadName(getState()) !== oldName) {
+    setDocTitle(getWorkpadName(getState()));


++ to checking the name changed before setting the doc title. This is a much better place to do the title update

elasticmachine · 2019-03-01T20:35:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…stic#31958) In an effort to remove the workpad renaming lag in elastic#25070, I started hunting down and fixing re-renders that happened with the name change. I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown: - `Workpad`, `WorkpadHeader`, and `FullscreenControl` would always re-render because the hotkey handler function was always re-created - `Workpad` would re-render when the workpad's name changed - We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes - Page title updating moved to middleware so the component doesn't need that value - `AssetManager` would always re-render if the parent re-rendered because it always created a new state value in `mapStateToProps` - make `Workpad` and `Toolbar` pure, they take no props and this helps stop some re-rendering

) (#32359) In an effort to remove the workpad renaming lag in #25070, I started hunting down and fixing re-renders that happened with the name change. I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown: - `Workpad`, `WorkpadHeader`, and `FullscreenControl` would always re-render because the hotkey handler function was always re-created - `Workpad` would re-render when the workpad's name changed - We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes - Page title updating moved to middleware so the component doesn't need that value - `AssetManager` would always re-render if the parent re-rendered because it always created a new state value in `mapStateToProps` - make `Workpad` and `Toolbar` pure, they take no props and this helps stop some re-rendering

) (#32358) In an effort to remove the workpad renaming lag in #25070, I started hunting down and fixing re-renders that happened with the name change. I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown: - `Workpad`, `WorkpadHeader`, and `FullscreenControl` would always re-render because the hotkey handler function was always re-created - `Workpad` would re-render when the workpad's name changed - We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes - Page title updating moved to middleware so the component doesn't need that value - `AssetManager` would always re-render if the parent re-rendered because it always created a new state value in `mapStateToProps` - make `Workpad` and `Toolbar` pure, they take no props and this helps stop some re-rendering

) (#32357) In an effort to remove the workpad renaming lag in #25070, I started hunting down and fixing re-renders that happened with the name change. I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown: - `Workpad`, `WorkpadHeader`, and `FullscreenControl` would always re-render because the hotkey handler function was always re-created - `Workpad` would re-render when the workpad's name changed - We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes - Page title updating moved to middleware so the component doesn't need that value - `AssetManager` would always re-render if the parent re-rendered because it always created a new state value in `mapStateToProps` - make `Workpad` and `Toolbar` pure, they take no props and this helps stop some re-rendering

…stic#31958) In an effort to remove the workpad renaming lag in elastic#25070, I started hunting down and fixing re-renders that happened with the name change. I was hoping this PR would fix the lag, but I've shifted to some other performance issues that have a much larger impact, so this is just a bunch of re-rendering fixes, which come with some performance gains. Here's the breakdown: - `Workpad`, `WorkpadHeader`, and `FullscreenControl` would always re-render because the hotkey handler function was always re-created - `Workpad` would re-render when the workpad's name changed - We were passing the whole workpad object into the component, so it re-rendered on all sorts of changes - Page title updating moved to middleware so the component doesn't need that value - `AssetManager` would always re-render if the parent re-rendered because it always created a new state value in `mapStateToProps` - make `Workpad` and `Toolbar` pure, they take no props and this helps stop some re-rendering (cherry picked from commit 7d8dbc6)

w33ble added WIP Work in progress Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Feb 25, 2019

w33ble requested a review from a team as a code owner February 25, 2019 21:06

w33ble force-pushed the fix/workpad-rename-lag branch from 3452677 to a6ecfad Compare February 25, 2019 21:07

w33ble force-pushed the fix/workpad-rename-lag branch from a6ecfad to a6e2ded Compare February 25, 2019 23:27

w33ble changed the title ~~Fix: workpad rename lag~~ Fix: Prevent some needless re-rendering in high-level components Feb 28, 2019

w33ble added review v7.0.0 v8.0.0 v6.7.0 v7.2.0 labels Feb 28, 2019

w33ble requested review from cqliu1, monfera and clintandrewhall February 28, 2019 17:09

w33ble added performance and removed WIP Work in progress labels Feb 28, 2019

w33ble added 8 commits February 28, 2019 14:44

chore: convert components to classes

e1b25b4

allow setting the keyhandler function on the component instance

fix: don't rebuild state when adding elements

f2dca08

fix: remove unused workpadName state val

f77bdad

fix: flatten workpad state, remove title change

628d349

flat state prevents re-renders, and not using the workpad name prevents re-renders. plus, side effects should happen in middleware

chore: update page title via middleware

2a0118d

chore: hoist fullscreen control handler

9146d4e

help prevent re-renders on function redefinition

fix: stop AssetManager from re-rendering

a773403

don't mutate values in mapStateToProps, so the connect function retains pure functionality

fix: make Toolbar and Workpad pure

82525ac

neither take props, so this stops some re-rendering

w33ble force-pushed the fix/workpad-rename-lag branch from 3168ff6 to 82525ac Compare February 28, 2019 21:47

w33ble mentioned this pull request Feb 28, 2019

Add http/1 response streaming to Canvas batches #32027

Merged

clintandrewhall approved these changes Feb 28, 2019

View reviewed changes

cqliu1 requested changes Feb 28, 2019

View reviewed changes

w33ble added 3 commits March 1, 2019 12:42

chore: remove console debugging

74d95ac

chore: replace magic number with constant

34d302d

chore: hoist conditions for clarity

0b85173

cqliu1 approved these changes Mar 1, 2019

View reviewed changes

w33ble merged commit 7d8dbc6 into elastic:master Mar 2, 2019

w33ble mentioned this pull request Mar 2, 2019

[7.x] Fix: Prevent some needless re-rendering in high-level components (#31958) #32357

Merged

w33ble mentioned this pull request Mar 2, 2019

[7.0] Fix: Prevent some needless re-rendering in high-level components (#31958) #32358

Merged

w33ble mentioned this pull request Mar 2, 2019

[6.7] Fix: Prevent some needless re-rendering in high-level components (#31958) #32359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Prevent some needless re-rendering in high-level components #31958

Fix: Prevent some needless re-rendering in high-level components #31958

w33ble commented Feb 25, 2019 •

edited

Loading

elasticmachine commented Feb 25, 2019

elasticmachine commented Feb 25, 2019

elasticmachine commented Feb 26, 2019

elasticmachine commented Feb 26, 2019

elasticmachine commented Feb 26, 2019

elasticmachine commented Feb 28, 2019

clintandrewhall Feb 28, 2019

w33ble Mar 1, 2019 •

edited

Loading

clintandrewhall Feb 28, 2019

w33ble Mar 1, 2019

cqliu1 left a comment

cqliu1 Feb 28, 2019

w33ble Mar 1, 2019

cqliu1 Feb 28, 2019

elasticmachine commented Mar 1, 2019

Fix: Prevent some needless re-rendering in high-level components #31958

Fix: Prevent some needless re-rendering in high-level components #31958

Conversation

w33ble commented Feb 25, 2019 • edited Loading

elasticmachine commented Feb 25, 2019

elasticmachine commented Feb 25, 2019

💚 Build Succeeded

elasticmachine commented Feb 26, 2019

💚 Build Succeeded

elasticmachine commented Feb 26, 2019

💔 Build Failed

elasticmachine commented Feb 26, 2019

💚 Build Succeeded

elasticmachine commented Feb 28, 2019

💚 Build Succeeded

clintandrewhall Feb 28, 2019

Choose a reason for hiding this comment

w33ble Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

clintandrewhall Feb 28, 2019

Choose a reason for hiding this comment

w33ble Mar 1, 2019

Choose a reason for hiding this comment

cqliu1 left a comment

Choose a reason for hiding this comment

cqliu1 Feb 28, 2019

Choose a reason for hiding this comment

w33ble Mar 1, 2019

Choose a reason for hiding this comment

cqliu1 Feb 28, 2019

Choose a reason for hiding this comment

elasticmachine commented Mar 1, 2019

💚 Build Succeeded

w33ble commented Feb 25, 2019 •

edited

Loading

w33ble Mar 1, 2019 •

edited

Loading