AudioParamTimeline leaks events causing page to become progressively slower
Reported by
brandon....@oculus.com,
Dec 13
|
||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.77 Safari/537.36 Steps to reproduce the problem: 1. Run attached html 2. See the dev console output 3. Note the time between frames is increasing rapidly What is the expected behavior? Time between frames should remain relatively stable. What went wrong? I noticed my VR game was slowing down, and tracked it to events leaking in AudioParamTimeline.cpp. I'm not familiar enough with this code to propose a fix. This likely impacts most 3d games/vr experiences that use Three.js to do positional audio. See: https://github.com/mrdoob/three.js/issues/15344 Did this work before? No Does this work in other browsers? N/A Chrome version: 70.0.3538.77 Channel: stable OS Version: 10.0 Flash Version: I've only tested as far back as m66, but it repros since then at least.
,
Dec 13
,
Dec 13
As per comment #0, Able to reproduce the issue on reported chrome 70.0.3538.77 and not seen on latest beta 72.0.3626.17 and latest chrome 73.0.3639.0 Using Windows 10. and issue not seen on M-60. Hence, Marking it as Untriage. Will provide the reverse bisect information and other OS behavior soon. @Reporter: can you please check and confirm the issue on latest chrome beta 72.0.3626.17, you can download latest chrome builds here:" https://www.chromium.org/getting-involved/dev-channel ". Let us know whether issue still persists. Thanks..!
,
Dec 13
The latest chrome beta (windows 10/64bit) shows version 71.0.3578.98 for me. I can confirm it doesn't no repro on this build.
,
Dec 13
Sorry I didn't mean the double negative in comment 5. It does not repro on the latest chrome beta.
,
Dec 13
If it doesn't repro in M71+, is there a way to point us out the commit that fixes it (via chromium-gerrit, or by just providing commit's SHA)?
,
Dec 13
The closest one I found is this: https://chromium-review.googlesource.com/c/chromium/src/+/1249583 This CL does not address the concern in #1. I have not investigated that yet. Let me know if you have more questions.
,
Dec 14
I doubt it, it doesn't seem to have anything with how events_ are allocated / processed (and I know for sure that the slow down was caused by growing events_ array inside the AudioParamTimeline::InsertEvent method. In fact, I've backported the whole new audio_param_timeline.cc from master into our version of the Browser (which is M66-based) and the leak / slowdown is still there. I've also went through the list of changes in WebAudio folder and I was unable to find the dedicated fix for this particular issue. This makes me think that the issue might be fixed as a part of other modifications, possibly inadvertently, i.e. it still may be a potential problem even though our repro doesn't demonstrate it anymore. Do you have any ideas why InsertEvent could be not adding more events in the case when source is stopped? Or, could it still add the events but for some reasons not iterating through the whole array, i.e. still leaking memory?
,
Dec 14
That is the only change in the range that is relevant to AudioParam's performance. > I've also went through the list of changes in WebAudio folder and I was unable to find the dedicated fix for this particular issue. That might be the case. You can try bisecting if you are really curious about the fix. The AudioParam code base has been managed by the other API owner, so it'll take some time for me in order to give you the better answer. I think the best course of action here is to close this one and open a new issue for a potential memory leak. If you can attach a simple repro code that affects M73 (canary), it would be great.
,
Dec 14
Able to reproduce the issue on reported chrome 70.0.3538.77 however issue seems to be fixed in latest chrome beta 72.0.3626.17 and canary 73.0.3638.0 using Mac 10.14.0, Windows 10 with steps mentioned in comment# 0, hence providing reverse bisect info Reverse Bisect Info: ================ Last Bad build: 72.0.3626.0 First Good build: 72.0.3626.10 As the issue break is seen in 72.0.3626 branch build, hence providing below change-log from omahaproxy CHANGELOG URL: https://chromium.googlesource.com/chromium/src/+log/72.0.3626.0..72.0.3626.10?pretty=fuller&n=10000 Reviewed-on: https://chromium-review.googlesource.com/c/1355333 Mounir Lamouri: Please confirm the issue and help in re-assigning if it is not related to your change.Adding 'ReleaseBlock-Stable' label as this is a recent regression. Please feel free to remove if this is not applicable. Note: Issue not observed on Ubuntu 17.10 Thanks..!
,
Dec 14
++Adding.. Abel to reproduce the issue on chrome stable M-71(71.0.3578.98) using Windows 10 and Mac 10.14.0. Updating the bisect info with nearest range and changing the priority=1. As the issue regressed in M-70,Hence removing ReleaseBlock-Stable M-71. Reverse Bisect Info: ================ Last Bad build: 72.0.3626.9 First Good build: 72.0.3626.10 Thanks..!
,
Dec 14
Okay, this might be a simple mistake. Please take a careful look at: https://developers.google.com/web/updates/2018/11/web-audio-autoplay The problem is the timeline is not being processed because the context is not running. The repro case does not follow the autoplay policy change (M71), so the context starts in the "suspended" state. The repro code is constantly scheduling new events on the suspended context. Thus events will just keep on piling. We can't reject scheduling, or remove events from the list without processing them, so the accumulating events is an expected behavior. There are certain points removing events from the list, so #1 is not true. https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc?sq=package:chromium&g=0&l=1067 https://cs.chromium.org/chromium/src/third_party/blink/renderer/modules/webaudio/audio_param_timeline.cc?sq=package:chromium&g=0&l=1907
,
Dec 14
phanindra@ Thanks for the bisect. It was really useful. Per #13, this is not a regression. I'll own this issue until reporters agree with my response. My plan is to close this issue "WontFix" after the agreement.
,
Dec 17
artyom17@ Could you take a look at The resolution at #14?
,
Dec 17
Well... Still weird that this repro is piling events in M66, doesn't do it in M70 and then again doing that in M72. Also, Firefox doesn't seem to do it. But it is up to you how to handle this issue, if you are certain that "it is by design" then it is fine.
,
Dec 17
> piling events in M66, doesn't do it in M70 and then again doing that in M72 Is it doing that again in M72? That's weird. What's the exact version number? So the events are piling up even when the context is running to process the graph?
,
Dec 17
I am sorry, I just got the impression from previous posts that the issue exists in M72 as well. Sorry for confusing, disregard my #16 message.
,
Dec 17
Don't be. I really appreciate you're raising this issue and helping the triage. As I mentioned in the related GitHub comment [1], the problem does not exist in the latest version of Chrome. I am closing this issue as WontFix, but please feel free to open a new issue if you find another performance issue. [1]: https://github.com/mrdoob/three.js/issues/15422#issuecomment-448013616
,
Dec 19
The NextAction date has arrived: 2018-12-19 |
||||||||
►
Sign in to add a comment |
||||||||
Comment 1 by artyo...@gmail.com
, Dec 13