phetsims / friction

"Friction" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/friction
GNU General Public License v3.0
4 stars 6 forks source link

Memory Leak #161

Closed KatieWoe closed 5 years ago

KatieWoe commented 5 years ago

For https://github.com/phetsims/QA/issues/244 Start: 18.7 MB 1 min: 35.4 MB 2 min: 48.3 MB 3 min: 60.0 MB 4 min: 72.3 MB 5 min: 82.6 MB 6 min: Memory tool crashes

zepumph commented 5 years ago

For a while I was unable to recreate such drastic numbers, then I realized that it was because my tab was muted. When unmuted I too get a huge memory leak. I looks to be related to Audio. I'll do a bit of poking, perhaps it is just a small disposal problem. I know @jbphet doesn't have a ton of time for Sonification right now. I'll toss it over to him if I need the help.

zepumph commented 5 years ago

I can't seem to consistently reproduce the crazy memory leak above. Sometimes when I refresh it hits 50MB within a minute, and sometimes (most of the time) it crawls up from 23 to 31MB over 30 minutes. I'll keep investigating.

This morning I haven't been able to get a large memory case yet.

image

The first 4 were over 10 minutes, then I kept refreshing with different audio conditions and seeing if things got big fast, (each snapshot was a different refresh after about 20 seconds of fuzz). I'll keep investigating

zepumph commented 5 years ago

All my previous testing was on chrome. On Firefox I can consistently get the startup memory footprint to be 50MB-55MB. But the weird part is that after 10 minutes or so it went down to 45MB.

UPDATE: Here is a screenshot of this. the yellow line is a refresh on the page. I don't yet have enough of an understanding of how muting effects these numbers. I know I was toggling muting in the tab throughout this, but never noticed a strong correlation. image

zepumph commented 5 years ago

Some more (same?) weirdness I think can be linked to whether or not the sound is muted on the tab.

This seems to me more difficult than I would expect to reproduce.

@KatieWoe could you please provide me with a bit more information.

You already told me that you used the simulation link. Does that mean you fuzzed with: https://phet-dev.colorado.edu/html/friction/1.5.0-rc.1/phet/friction_en_phet.html?fuzz

What device and browser did you use?

Was the tab playing sound? Did you click in the window to start sound? Was the tab muted? Was the computer muted?

Thanks!

KatieWoe commented 5 years ago

@zepumph Fuzzed with: https://phet-dev.colorado.edu/html/friction/1.5.0-rc.1/phet/friction_en_phet.html?fuzz Dell laptop Chrome I believe that chrome muted the sound itself, I did not click to start sound or do anything to stop it myself. I don't think my computer was muted.

jbphet commented 5 years ago

I believe that chrome muted the sound itself

Chrome's relatively recent autoplay policy doesn't allow sound to be played on a web page unless the user interacts with that page. There are some exceptions, but that's basically how it works as of version 71.

jbphet commented 5 years ago

@zepumph and I looked at this together, and we think we know what's up. The 'autoPlay' policy change means that the audio context isn't running until a user interacts with the sim. During fuzz testing, no true user interaction occurs, so the audio context never gets enabled. However, the internally generated user events produced by the fuzz testing cause a lot of things to happen that try to produce sounds. These events end up allocating AudioSourceBufferNode instances that never get to play, so they never finish, so they never get garbage collected. Ergo, memory leak.

I was recently experimenting with some code (for other reasons) that prevented these allocations if the audio context isn't enabled. I'll finalize this and move it to master, and I believe that will resolve the problem.

It's worth noting that this is not something that would be a problem for our users, it's only an issue for fuzz testing. Still, memory leaks during fuzz testing could mask other issues, so it needs to be corrected.

jbphet commented 5 years ago

I've added deferred play and start for some of the sound generators so that they aren't creating new audio source buffers when the audio context is suspended, since this is where the bulk of the memory leak comes from. I just retested, and there is a vast improvement, but I am still seeing some leakage of audio source buffers in some circumstances. I'll need to do a bit more on this before it can be considered fully fixed.

zepumph commented 5 years ago

@jbphet do you think that this should be picked into Friction's release branch, if so please give me shas to do so with.

jbphet commented 5 years ago

I just did some memory profile testing, and the details are logged below. My goal was to determine if memory leaks were still occurring as a result of the sound library. The bottom line is that I don't think there are any such memory leaks occurring after the latest changes. There do appear to be some slow memory leaks still happening, but I feel that these are very unlikely to cause any user issues. I'll leave it to @zepumph to decide if we should continue to pursue these leaks. As you'll see below, they appear to be in the framework code.

Below is a screenshot for a test where the master RequireJS version is loaded into an incognito window. This gives us the baseline memory usage prior to any interaction:

image

Next is a screenshot of a test where the sim is started with the fuzz query parameter, then a heap snapshot is taken at the 1, 2, 3, 4, 5, and 6 minute marks, then fuzz testing is stopped using the command phet.chipper.queryParameters.fuzz=false, a heap snapshot is taken, then the reset button is pressed (which is the first user interaction), and a snapshot is taken again. There is slow growth in the amount of memory, but nothing that comes close to crashing the browser or causing performance problems. I did several comparisons, and there were none that appeared to show sound-library-related memory leakage. The Utterance leakage might be worth pursuing, but it seems pretty minimal.

image

For the following image, I started the sim with the fuzz parameter, let it run for 30 seconds, then stopped it and reset it and took a heap snapshot, then restarted fuzz for one minute, stopped, reset, and took a snapshot. This seems like the most legit test in terms of vigorous user interaction. Again, I'm not seeing any signs of leakage from sound library components. I'm seeing a number of what looks like leaked items that trace back to the UpdateDialog, such as the AccessibleDisplaysInfo object shown in the screen capture, so that might be worth pursuing. But again, this isn't a level of leakage that would ever imperil the operation of the sim during typical usage.

image

jbphet commented 5 years ago

@zepumph - you asked for the commits that fix the sound-related memory leak. There are a number of them, and they are also related to another issue with sound generation, see https://github.com/phetsims/friction/issues/159. I can list them if you like, but I think the approach that is most likely to be successful is for you to let me know when we're ready to roll the next RC, and I'll branch tambo and integrate the changes and test locally, since this is going to be a bit tricky. I'll also pull in the related commits for the friction release branch. Assigning to you for feedback on this plan.

zepumph commented 5 years ago

This was discussed during shorts meeting on Tuesday. We decided that @jbphet will cherry-pick the appropriate commits to friction. @jbphet please assign back to me when things are ready to be published.

jbphet commented 5 years ago

I've propagated the fixes to the Friction release branch, and testing shows the same things reported above, which is that the sound-related memory leaks appear to be fixed but there is still a slow leak coming from somewhere. Back to you @zepumph for the next steps.

zepumph commented 5 years ago

Taken from over 10 minutes of fuzzing a locally built version of the 1.5 branch on chrome, it seems to be fixed: image

Note, the key here is to make sure that the audio context is not allowed to start, for example seeing messages like Audio context not allowed to start are good to reproduce the potentially buggy case.

zepumph commented 5 years ago

Unfortunately this appears to be broken on IE, to reproduce (on master or friction-1.5 branch):

you should get this error while loading: "Object doesn't support property or method 'linearRampToValueAtTime'"

marking high priority because this is holding up the rc at this point.

jbphet commented 5 years ago

This should now be fixed - I needed to add a new method to the stubbed audio context that is used when running on Internet Explorer. Back to you @zepumph.

zepumph commented 5 years ago

Thanks!

KatieWoe commented 5 years ago

@zepumph New results for 1.5.0-rc.2. I clicked on the sim to start sound to avoid this issue entirely, but I'm running a separate test as well to check the fix. Start: 18.7 1 min: 25.6 2 min: 26.1 3 min: 26.7 4 min: 27.0 5 min: 27.3 6 min: 27.8 7 min: 27.7 8 min: 27.9 9 min: 28.1 10 min: 28.4

The results from the test in which I did not start the sound up: Start 18.7 After about 5 min: 27.1