Skip to content

fix: prevent memory leak in Visualizer component #114

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

Merged
merged 4 commits into from
Jan 23, 2021

Conversation

huyenltnguyen
Copy link
Member

@huyenltnguyen huyenltnguyen commented Jan 1, 2021

This PR:


The issue can be reproduce by making the following changes to the code:

  • Make the Main component render the Visualizer conditionally based on the playing state
  • Add a console.log statement to this line

Expected behavior: When the user presses pause, the Visualizer component is unmounted, and there shouldn't be anything logged to the browser console.
Actual behavior: When the user presses pause, the Visualizer component is unmounted, and there are logs constantly printed in the browser console.


My fix for this is to move the recursion outside of the draw function, and implement start and stop functions that helps initiate and clean up animation loops instead.

References:

@huyenltnguyen huyenltnguyen force-pushed the fix/prevent-memory-leak branch 2 times, most recently from 8dcecdd to 0d1c658 Compare January 10, 2021 09:58
@huyenltnguyen huyenltnguyen changed the title [WIP] fix: prevent memory leak in Visualizer component fix: prevent memory leak in Visualizer component Jan 10, 2021
@huyenltnguyen huyenltnguyen marked this pull request as ready for review January 10, 2021 10:05
@huyenltnguyen huyenltnguyen requested a review from ahmaxed January 10, 2021 10:07
@huyenltnguyen
Copy link
Member Author

I haven't been able to intensively test the CPU usage, but I do hope that it reduces some 🤞

@huyenltnguyen huyenltnguyen force-pushed the fix/prevent-memory-leak branch from 0d1c658 to 607c1ef Compare January 10, 2021 11:00
@ahmaxed
Copy link
Member

ahmaxed commented Jan 11, 2021

These are the errors I get after playing/pausing and playing again
Screen Shot 2021-01-11 at 1 52 30 PM

@huyenltnguyen
Copy link
Member Author

Hmm. I'm guessing that the AudioContext didn't finish resetting at the time we pressed play again.

I'm actually thinking about keeping the Visualizer in the DOM when the music is paused or when the user switches tab, but stop the draw function instead, so that we won't have to reinitialize the audio context again. I think this approach will help keep the animation transition smooth.

@huyenltnguyen huyenltnguyen force-pushed the fix/prevent-memory-leak branch from 0606504 to 86bd752 Compare January 16, 2021 12:42
@huyenltnguyen huyenltnguyen force-pushed the fix/prevent-memory-leak branch from 15279bb to f83348c Compare January 16, 2021 17:45
@huyenltnguyen
Copy link
Member Author

@ahmadabdolsaheb I found that apparently ifvisible.js has a memory leak issue (serkanyersen/ifvisible.js#50), so I decided to use react-page-visibility instead.

This PR is ready for another round of review 🙂

@ahmaxed ahmaxed force-pushed the fix/prevent-memory-leak branch 3 times, most recently from 9c1e4a6 to f83348c Compare January 20, 2021 08:35
if (nextProps.playing && !this.state.eq.context) {
this.initiateEQ();
componentDidUpdate(prevProps, prevState) {
if (prevProps.playing === this.props.playing && prevState.isTabVisible === this.state.isTabVisible) {
Copy link
Member

Choose a reason for hiding this comment

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

should we do (prevProps.playing !== this.props.playing && prevState.isTabVisible !== this.state.isTabVisible) so all of the conditions can stay in one block.

Comment on lines +8 to +9
rafId = null;
timerId = null;
Copy link
Member

Choose a reason for hiding this comment

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

Since they are changing, should we set the to states?

Copy link
Member

Choose a reason for hiding this comment

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

not sure what rafId stands for.

@ahmaxed
Copy link
Member

ahmaxed commented Jan 20, 2021

Thanks for the pr. see my nitpicks above, other than that LGMT

@ahmaxed ahmaxed merged commit 83a3e3a into freeCodeCamp:master Jan 23, 2021
@huyenltnguyen huyenltnguyen deleted the fix/prevent-memory-leak branch January 30, 2021 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants