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

Fix Shortcut EventEmitter Leak/Re-render leaks #31779

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

clintandrewhall
Copy link
Contributor

@clintandrewhall clintandrewhall commented Feb 22, 2019

Summary

We were seeing EventEmitter leaks involving Shortcuts, and my analysis of unnecessary re-renders showed Shortcuts was repainting excessively, (more than 300 times on Workpad load).

I discovered this is due to the Shortcuts element being rendered on every page of the workpad... I added a check to be sure the page was active, and that solved the ventEmitter problem.

I then extracted out the logic into a standalone component and created bound functions in that component to prevent unnecessary repaint. Since the selectedElements and selectedElementIds are being recreated with each aeroelastic change, I also added a fast-check of the props in the new WorkpadShortcuts component to prevent repaints.

In all, this change fixes the steady increase in listeners being attached and GC'd, and drastically improves re-render on the main thread.

Listeners being created and GC'd, before, 365-391:
screen shot 2019-02-22 at 12 39 33 am

After, 16-130:
screen shot 2019-02-22 at 12 43 00 am

@clintandrewhall clintandrewhall added review v7.0.0 performance Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.7.0 labels Feb 22, 2019
@clintandrewhall clintandrewhall requested a review from a team as a code owner February 22, 2019 06:44
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-canvas

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

Fantastic PR, brings the direct manipulation interaction speed to 30-60FPS with a dozen or two elements, all interactions still work and nice extractions too!

@monfera monfera merged commit 923f360 into elastic:master Feb 22, 2019
monfera pushed a commit to monfera/kibana that referenced this pull request Feb 22, 2019
monfera pushed a commit to monfera/kibana that referenced this pull request Feb 22, 2019
monfera pushed a commit to monfera/kibana that referenced this pull request Feb 22, 2019
monfera pushed a commit to monfera/kibana that referenced this pull request Feb 22, 2019
monfera added a commit that referenced this pull request Feb 22, 2019
monfera added a commit that referenced this pull request Feb 22, 2019
@clintandrewhall
Copy link
Contributor Author

Thanks @monfera ... I had a nasty case of the flu Friday and lost track of this guy. You rock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v6.7.0 v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants