-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Kill unnecessary re-renders in ElementWrapper #31734
Kill unnecessary re-renders in ElementWrapper #31734
Conversation
Pinging @elastic/kibana-canvas |
💔 Build Failed |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, great change!
There's some minor yarn.lock
conflicts due to earlier merges.
Apparently unrelated to this PR or your preceding PR (EventEmitters leak), but I saw this just now, after doing some crazyman drag&drop testing (15-20 shapes and one uploaded image being dragged and dropped prolifically): <= @w33ble just told me this warning has been around for a while when an image is present, so pls. ignore this from the angle of this PR
Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in withState(withState(lifecycle(Connect(branch(ArgFormComponent))))) (created by withState(withState(withState(lifecycle(Connect(branch(ArgFormComponent)))))))
in withState(withState(withState(lifecycle(Connect(branch(ArgFormComponent)))))) (created by FunctionFormComponent)
in div (created by EuiPanel)
in EuiPanel (created by SidebarSection)
in SidebarSection (created by FunctionFormComponent)
in div (created by FunctionFormComponent)
in FunctionFormComponent (created by branch(FunctionFormComponent))
in branch(FunctionFormComponent) (created by branch(branch(FunctionFormComponent)))
in branch(branch(FunctionFormComponent)) (created by branch(branch(branch(FunctionFormComponent))))
in branch(branch(branch(FunctionFormComponent))) (created by Connect(branch(branch(branch(FunctionFormComponent)))))
in Connect(branch(branch(branch(FunctionFormComponent)))) (created by FunctionFormList)
FYI, when I try to merge master and bootstrap to fix the yarn conflict, I get a build error on some types in the i18n package. No idea what's up with that, but the conflict might be tricky to fix because of it... |
Wondering how it can happen as this PR seems to have no i18n aspect on a superficial look. Maybe rebasing it on current master would fix whatever glitch is preventing it. Btw. I saw it in full green (no merge conflict, and CI passed), no idea if me merging Clint's other PR caused the merge conflict or build issue, or something unrelated that went in during that approx. one hour (other folks merged to master too). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, but this looks good. The app seems to load a big faster, and interactions seem smoother too. I'm not sure how much of that is placebo, but I'll take it 😉.
I was hoping that this might fix #25070, but no dice, it's still really laggy. All the same, this is a win.
__ ___ _____ _
/ / / _ \/__ \/\/\ / \
/ / / /_\/ / /\/ \ / /
/ /___/ /_\\ / / / /\/\ \ /\_/
\____/\____/ \/ \/ \/ \/
x-pack/package.json
Outdated
@@ -130,6 +130,7 @@ | |||
"ts-loader": "^5.2.2", | |||
"typescript": "^3.0.3", | |||
"vinyl-fs": "^3.0.2", | |||
"why-did-you-update": "^1.0.6", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this in the package? You're not using it anywhere, and it seems easy enough to only pull it in locally when you want to diagnose re-renders.
@clintandrewhall @w33ble since it's a personal feature branch, I can't push a commit to it (I contemplated merging it haha), but here's what I did:
|
... looks like we have this perf enhancement opportunity in 24 other components that use |
Most of the |
I'm already investigating more improvements... I'll keep shipping them as I find them! See #31779. |
💚 Build Succeeded |
## 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)~~
## 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)~~
## 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)~~
## Summary (fixes #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)~~
## Summary (fixes #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)~~
Summary (fixes #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):
Here is a snapshot after. That reconciliation now completes in 50ms or less:
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
strikethroughsto remove checklist items you don't feel are applicable to this PR.Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This includes a feature addition or change that requires a release note and was labeled appropriately