Chromecast sometimes running out of memory after seeking |
||||||
Issue description
Version: 52
OS: Linux (Chromecast)
Still working on a simple and reliable repro. Initial report is based on the fact that memory usage often spikes after seeking, and in some situations can lead to complete OOM (e.g. render process killed by linux oom killer).
Current suspicion is that XHR requests are the major contributing factor. In this simple script we see system available memory drop by about 3.5x the size of the resource fetched:
window.onload = function() {
var req = new XMLHttpRequest();
req.onload = function(e) {
var arraybuffer = req.response;
console.log('!!!LJH got response of length: ' + arraybuffer.byteLength);
}
req.responseType = 'arraybuffer';
req.open('GET', <media-url>);
req.send();
}
Some additional Google-internal detail is available on b/32447172
,
Oct 26 2016
We have a slightly slower release cycle that means we're always a little behind. And right now we're even further than normal because of needing a longer stabilisation period for new hardware launch. We'll catch up at our next release, but there is nothing we can do about this in the immediate short term.
,
Oct 26 2016
Question: why does an XHR request use 2.5-3.5x the memory of the resource it's fetching? Are there expected to be multiple copies of the data? Are there any options to reduce this?
,
Oct 26 2016
It does use double the memory inside the .response getter when we copy the data from the SharedBuffer into the DOMArrayBuffer. We clear the SharedBuffer after that though so it's just momentary. If you can reproduce this on M56 we can look into fixing it. I don't we can help you with M52 though, you'll have to study the code and consider ways to hack it up on your branch.
,
Oct 26 2016
Thanks Elliott. I totally understand that you're not going to spend time debugging M52 for us :) Whenever we're in this situation, our team definitely expects to do most of the work and potentially make local fixes ourselves. But we really depend on getting advice or hints on where to look or what to try from experts on the relevant code. When you say "we clear the SharedBuffer after that", where does that happen? In Resource's destructor? (I don't see any other calls, but correct me if I'm missing something). Is it deterministic or driven by GC?
,
Oct 26 2016
That's deterministic: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp?q=XMLHttpRequest&sq=package:chromium&l=394 the XMLHttpRequest is keeping the data in a SharedBuffer as it streams in from didReceiveData. Inside the getter method for .response (which returns the ArrayBuffer) we copy into the ArrayBuffer and then do m_binaryResponseBuilder.clear() which releases the SharedBuffer. It does look like the MemoryCache might also keep the Resource alive, but I'll admit I don't actually understand that end of the system and why we can't pipe the memory cache data directly into the ArrayBuffer. The object we return to JS is mutable though, so we'll always need at least two copies if we're using the MemoryCache for XHR resources. +kinuko@, kouhei@ who both understand the MemoryCache better than me.
,
Oct 26 2016
Btw can you attach a self contained repro we could test on Canary?
,
Oct 26 2016
,
Oct 26 2016
Hmmm, the shared buffer I was looking at is Resource::m_data: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/fetch/Resource.h?q=resource::m_data&sq=package:chromium&l=466 So it looks like there actually *two* copies being built up during the fetch (Resource's m_data and XHR's m_binaryResponseBuilder), is that right? And that Resource is to do with caching ... I wonder if there are options to tune the cache behaviour to avoid that copy in our case? Totally happy to put together a self-contained repro, although I don't understand what this failure case would look like on desktop Chrome without understanding the problem better.
,
Oct 26 2016
Your test case should show a 2.5-3.5x memory usage per XHR response. Looking at the code I think if you do: resourceLoaderOptions.dataBufferingPolicy = DoNotBufferData; for responseType() of ArrayBuffer we'll not create the m_data inside Resource, we do that for Blob today. That does mean you lose the memory cache for ArrayBuffer responses and fall back on the disk cache. I think stable (M56) also has better treatment in general inside the MemoryCache where those resources are stored weakly, and the tuned GC herustics should mean we clean up the duplicate data more quickly. In the long term we should probably change the loading code so XHR gets the SharedBuffer directly out of the MemoryCache instead of creating a copy inside didReceiveData. The loading pipeline for (network || disk) -> XHR ArrayBuffer seems like it could be optimized a bunch. :)
,
Oct 27 2016
Afaik XHR and memory cache have been having several problems (though my knowledge could be getting a bit stale), I hoped we could just set DoNotBufferData and disable memory cache for xhr but it might have perf impact? Iirc it seems short-term memory surge while receiving xhr will be around even with weak memory cache. Adding more experts for xhr+memory cache
,
Oct 27 2016
> but it might have perf impact? Not really, I believe. As in issue 399166 , a memory cache entry for XHR will be removed after load finishes. I'm for specifying DoNotBufferData.
,
Oct 27 2016
Is this bug still happening on Canary? We have landed a couple of fixes to ArrayBufferContents recently.
,
Oct 27 2016
Yes, it is still happening. I just synced up to head this morning to verify. It seems the bug is not connected to ArrayBufferContents, but to the memory caching of XHR ArrayBuffer requests. For a repro, it's easiest to see with a big network fetch to drown out any other noise. You can just use the script snippet that I put in the bug description and set the URL to a suitably large resource. I attached a couple of memory-infra traces of running this snippet fetching a ~100MB file. With latest Chrome, after the download finishes, the render process has: * Total resident = 138MB * Peak total resident = 344MB Then I applied this change to disable buffering for XHR ArrayBuffer: https://codereview.chromium.org/2460573002 The trace with this change shows: * Total resident = 138MB * Peak total resident = 239MB It seems we do indeed reduce peak memory usage by the entire 100MB.
,
Nov 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0b83d55ccf3742ac2f849c33c3db796230d9cf9c commit 0b83d55ccf3742ac2f849c33c3db796230d9cf9c Author: yhirano <yhirano@chromium.org> Date: Wed Nov 16 09:44:43 2016 Do not add a Resource with DoNotBufferData policy to the memory cache BUG= 659789 Review-Url: https://codereview.chromium.org/2509533002 Cr-Commit-Position: refs/heads/master@{#432448} [modify] https://crrev.com/0b83d55ccf3742ac2f849c33c3db796230d9cf9c/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
,
Nov 16 2016
,
Nov 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e3207a5d277b089f5ea868fa402b8f703bdc830c commit e3207a5d277b089f5ea868fa402b8f703bdc830c Author: halliwell <halliwell@chromium.org> Date: Wed Nov 16 15:58:02 2016 Disable memory caching of XHRs XHRs get removed from memory cache anyway after load finishes (see issue 399166 ). This change can save significant memory during the fetch for large resources. BUG= 659789 Review-Url: https://codereview.chromium.org/2460573002 Cr-Commit-Position: refs/heads/master@{#432502} [modify] https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html [modify] https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp [modify] https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
,
Nov 16 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e3207a5d277b089f5ea868fa402b8f703bdc830c commit e3207a5d277b089f5ea868fa402b8f703bdc830c Author: halliwell <halliwell@chromium.org> Date: Wed Nov 16 15:58:02 2016 Disable memory caching of XHRs XHRs get removed from memory cache anyway after load finishes (see issue 399166 ). This change can save significant memory during the fetch for large resources. BUG= 659789 Review-Url: https://codereview.chromium.org/2460573002 Cr-Commit-Position: refs/heads/master@{#432502} [modify] https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/reload-memory-cache.html [modify] https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp [modify] https://crrev.com/e3207a5d277b089f5ea868fa402b8f703bdc830c/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
,
Nov 16 2016
[Bulk edit] Branch issues caused these items to be erroneously tagged as merged to branch 2922, please ignore.
,
May 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a commit 35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a Author: allada <allada@chromium.org> Date: Wed May 03 00:06:03 2017 [Devtools] Fixed resource having no content if from xhr blob Offending patch: https://codereview.chromium.org/2460573002/ In the event that a resource is an xhr request and the data had been saved to a file and had an error code in the header it was not showing any content uppon request for body. R=dgozman,caseq BUG= 687677 , 659789 Review-Url: https://codereview.chromium.org/2848353003 Cr-Commit-Position: refs/heads/master@{#468827} [add] https://crrev.com/35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-fetch-content-with-error-status-code-expected.txt [add] https://crrev.com/35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network-fetch-content-with-error-status-code.html [add] https://crrev.com/35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/cors-data.php [modify] https://crrev.com/35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/resources/protocol-test.html [modify] https://crrev.com/35aaf5a9b46c7464901e6dab4c8c947cf9f60c7a/third_party/WebKit/Source/core/inspector/InspectorNetworkAgent.cpp |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by esprehn@chromium.org
, Oct 26 2016