Skip to content

Commit

Permalink
Kill unnecessary re-renders in ElementWrapper (elastic#31734)
Browse files Browse the repository at this point in the history
## Summary (fixes elastic#29700)

This is a small change that creates a huge impact on UI performance in Canvas.

As the mouse moves over a workpad, several parts of the state of the workpad, (the element hovered, the element moved if one is being held, etc) change.  These changes in state were causing every active Element to re-render in React.

In short, you may hover Element 10, but Elements 1-9 re-render because the state of the entire workpad changes.  This would happen even if the mouse were moved a single pixel.

By short-circuiting that re-render with a fast comparison, we prevent any Elements that are not affected from re-rendering... freeing up the main thread.  This performance impact intensifies as one adds Elements to the workpad.

Here is a performance shapshot before this change was made.  Notice the React tree reconciliation encompasses the entire 280ms.  This reflects a single state change flowing through all of the Elements on the workpad, (each orange "stalactite" descending downwards is an Element):

![screen shot 2019-02-21 at 11 49 48 am](https://user-images.githubusercontent.com/297604/53197762-76960380-35e0-11e9-9069-a30df5fd4b2f.png)

Here is a snapshot after.  That reconciliation now completes in 50ms or less:

![screen shot 2019-02-21 at 11 49 30 am](https://user-images.githubusercontent.com/297604/53197845-afce7380-35e0-11e9-8991-572462b441eb.png)

*This is not the end of this problem.*  There are many other opportunities to prevent re-rendering, and we need to focus on killing the need for the comparison in the first place.  @monfera and @w33ble will be instrumental in making these changes in the future... for now, though, this gets the job done.

### Checklist

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

- [x] 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

- [x] 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)~~
  • Loading branch information
clintandrewhall committed Feb 25, 2019
1 parent d1acfe5 commit beadb1b
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,28 +19,19 @@ export class ElementWrapper extends React.PureComponent {
createHandlers: PropTypes.func.isRequired,
};

state = {
handlers: null,
};

componentDidMount() {
// create handlers when component mounts, so it only creates one instance
const { createHandlers } = this.props;
// eslint-disable-next-line react/no-did-mount-set-state
this.setState({ handlers: createHandlers() });
constructor(props) {
super(props);
this._handlers = props.createHandlers(props.selectedPage);
}

render() {
// wait until the handlers have been created
if (!this.state.handlers) {
return null;
}
_handlers = null;

render() {
const { renderable, transformMatrix, width, height, state } = this.props;

return (
<Positionable transformMatrix={transformMatrix} width={width} height={height}>
<ElementContent renderable={renderable} state={state} handlers={this.state.handlers} />
<ElementContent renderable={renderable} state={state} handlers={this._handlers} />
</Positionable>
);
}
Expand Down
24 changes: 16 additions & 8 deletions x-pack/plugins/canvas/public/components/element_wrapper/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,20 @@

import PropTypes from 'prop-types';
import { connect } from 'react-redux';
import { getResolvedArgs, getSelectedPage, isWriteable } from '../../state/selectors/workpad';
import { compose, shouldUpdate } from 'recompose';
import isEqual from 'react-fast-compare';
import { getResolvedArgs, getSelectedPage } from '../../state/selectors/workpad';
import { getState, getValue, getError } from '../../lib/resolved_arg';
import { ElementWrapper as Component } from './element_wrapper';
import { createHandlers as createHandlersWithDispatch } from './lib/handlers';

const mapStateToProps = (state, { element }) => ({
isWriteable: isWriteable(state),
resolvedArg: getResolvedArgs(state, element.id, 'expressionRenderable'),
selectedPage: getSelectedPage(state),
});

const mapDispatchToProps = (dispatch, { element }) => ({
createHandlers: pageId => () => createHandlersWithDispatch(element, pageId, dispatch),
createHandlers: pageId => createHandlersWithDispatch(element, pageId, dispatch),
});

const mergeProps = (stateProps, dispatchProps, ownProps) => {
Expand All @@ -27,6 +28,7 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
const { id, transformMatrix, width, height } = element;

return {
selectedPage,
...restProps, // pass through unused props
id, //pass through useful parts of the element object
transformMatrix,
Expand All @@ -35,14 +37,20 @@ const mergeProps = (stateProps, dispatchProps, ownProps) => {
state: getState(resolvedArg),
error: getError(resolvedArg),
renderable: getValue(resolvedArg),
createHandlers: dispatchProps.createHandlers(selectedPage),
createHandlers: dispatchProps.createHandlers,
};
};

export const ElementWrapper = connect(
mapStateToProps,
mapDispatchToProps,
mergeProps
export const ElementWrapper = compose(
connect(
mapStateToProps,
mapDispatchToProps,
mergeProps,
{
areOwnPropsEqual: isEqual,
}
),
shouldUpdate((props, nextProps) => !isEqual(props, nextProps))
)(Component);

ElementWrapper.propTypes = {
Expand Down

0 comments on commit beadb1b

Please sign in to comment.