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: Move construct super to fix memory leak when using multiple withThemeStyles #456

Merged
merged 3 commits into from
Jan 9, 2024

Conversation

joshhowenstine
Copy link
Contributor

@joshhowenstine joshhowenstine commented Jan 8, 2024

Issue

This pull request addresses a memory leak problem identified when the withThemeStyles mixin is utilized repeatedly. It was revealed that multiple initializations were contributing to this memory leak. To resolve this, I have modified the mixin's implementation by relocating the super._construct() call to the end of the process. This change ensures that super._construct() is only executed once, thereby preventing the repetitive initializations that were causing the memory leak.

Testing

  • To thoroughly test the modifications made to the withThemeStyles mixin and confirm the resolution of the memory leak issue, follow these detailed steps:

Component Wrapping:

  • Start by wrapping a component, such as Tile, with the withThemeStyles mixin. Considering that Tile already incorporates withThemeStyles, this action results in its multiple applications. You should be able to do this in the Tile story.

Setup a Repetitive Create and Destroy Cycle:

  • Establish a routine where the wrapped Tile component is continuously created and then destroyed. This can be done by setting an interval in your code that alternates between creating and removing the component.

Open Chrome DevTools:

  • Launch your application in Google Chrome.

  • Open Chrome DevTools

Go to the Memory Tab:

I- [ ] n DevTools, navigate to the “Memory” tab. This tab provides tools for analyzing runtime memory usage.
Take an Initial Heap Snapshot:

  • Click on the “Take snapshot” button to capture a heap snapshot before the interval starts. This serves as a baseline

Run the Create-Destroy Cycle:

  • Execute the interval you set up for creating and destroying the Tile component. Let it run for a sufficient amount of time.

Take Another Heap Snapshot:

  • After running the interval for some time, take another heap snapshot. This snapshot will help you analyze the memory allocation after multiple create-destroy cycles.

Compare Heap Snapshots:

  • Analyze the two snapshots by comparing them. Look for discrepancies in memory usage and check for any retained memory that might indicate a leak. Specifically, focus on the listeners and related objects.

Repeat with and without the Change:

  • Perform this test once without the change (using the original code) and once with the change. This will help you clearly see the impact of the modification.

@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Passed.

2 similar comments
@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Passed.

@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Passed.

@ImCoolNowRight ImCoolNowRight force-pushed the fix/with-theme-styles-mem branch from 9d335f1 to 60aa26d Compare January 9, 2024 20:51
@svc-lightning-ui-components
Copy link
Collaborator

Test Execution Passed.

@ImCoolNowRight ImCoolNowRight merged commit e983f17 into develop Jan 9, 2024
9 checks passed
@ImCoolNowRight ImCoolNowRight deleted the fix/with-theme-styles-mem branch January 9, 2024 21:22
svc-lightning-ui-components pushed a commit that referenced this pull request Jan 10, 2024
# [@lightningjs/ui-components-v2.19.6](https://github.com/rdkcentral/Lightning-UI-Components/compare/@lightningjs/ui-components-v2.19.5...@lightningjs/ui-components-v2.19.6) (2024-01-10)

### Bug Fixes

* **CardTitle:** updating Input and CardTitle styles to resemble mosaic ([#454](#454)) ([3f9ab40](3f9ab40))
* **Import:** standardize how components are imported ([#455](#455)) ([5f7ce24](5f7ce24))
* Move construct super to fix memory leak when using multiple withThemeStyles ([#456](#456)) ([e983f17](e983f17))
* **Slider:** adds intial content so Value shows on load ([#451](#451)) ([5e00f9b](5e00f9b))
* **Tile:** update conditions to show/hide gradient ([#457](#457)) ([31514cd](31514cd))
* **Tooltip:** adds updateBackground to _update ([#434](#434)) ([63ee073](63ee073))
@svc-lightning-ui-components
Copy link
Collaborator

🎉 This PR is included in version @lightningjs/ui-components-v2.19.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants