Summary: | [V8] OOM in Workers due to external memory retention. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dmitry Titov <dimich> | ||||||||
Component: | WebCore Misc. | Assignee: | Dmitry Titov <dimich> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, antonm, dglazkov, haraken, japhet, levin, levin+threading, peter+ews, webkit.review.bot, yurys | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Dmitry Titov
2012-09-11 19:29:55 PDT
Created attachment 163500 [details]
Proposed patch.
Comment on attachment 163500 [details] Proposed patch. Actually, how does the static "workingSetEstimateMB" function in the case of workers? http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/v8/V8GCController.h&exact_package=chromium&q=workingSetEstimateMB&type=cs&l=84 And how does the supporting code in Chromium work in the case of threading? When I see caching on a static object like here (http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webkitplatformsupport_impl.cc&exact_package=chromium&q=memoryUsageMB&type=cs&l=728) it concerns me. Thanks David! Here is what I think happens there: The static MemoryUsageCache has locks in its implementation, and its usage looks technically valid. It caches the current overall process usage for 1 second, to reduce the frequency of querying. The rest of the memory usage code in webkitplatformsupport_impl.cc also looks workable in threading case except a non thread safe static initialization in here: http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webkitplatformsupport_impl.cc&exact_package=chromium&q=MemoryUsageCache&l=687. I will need a Chrome-side patch as well. Static workingSetEstimateMB works fine except in case when in-process dedicated workers also are present in the process. In this case, it'll capture somewhat random (higher) working set estimate because it can be captured for a GC happening in any of the dedicated workers. It can cause less-aggressive GCs when current usage is in range between lowMemoryUsageMb and highMemoryUsageMB. That seems ok given the very approximate behavior of the heuristic in that range: it uses memory values cached for a second, assumes non-v8 working set is stable and starts to aggressively GC if the v8 heap grows more then 2 times larger then the non-v8 one... So I think it will work in multi-threaded sense, with heuristic causing less aggressive GCs when dedicated workers are present, which is probably ok. I wasn't able to find the initial motivation for the specific heuristic, hopefully Anton or Adam can chime in. Regarding heuristics. No, it doesn't assume that no v8-heap is stable, on the contrary, it should kick in if allocation rate in v8 heap is too low to trigger major GC while non-v8 heap grew notably and hence there is a change that lots of "C++" memory is retained by thin v8 wrapper. All that said, I don't see immediate problems with extending this heuristics to workers. But most probably you need to ensure that reads and writes to workingSetEstimateMB are atomic. (In reply to comment #3) > Thanks David! > > Here is what I think happens there: > > The static MemoryUsageCache has locks in its implementation, and its usage looks technically valid. It caches the current overall process usage for 1 second, to reduce the frequency of querying. The rest of the memory usage code in webkitplatformsupport_impl.cc also looks workable in threading case except a non thread safe static initialization in here: http://code.google.com/searchframe#OAMlx_jo-ck/src/webkit/glue/webkitplatformsupport_impl.cc&exact_package=chromium&q=MemoryUsageCache&l=687. I will need a Chrome-side patch as well. > > Static workingSetEstimateMB works fine except in case when in-process dedicated workers also are present in the process. In this case, it'll capture somewhat random (higher) working set estimate because it can be captured for a GC happening in any of the dedicated workers. It can cause less-aggressive GCs when current usage is in range between lowMemoryUsageMb and highMemoryUsageMB. That seems ok given the very approximate behavior of the heuristic in that range: it uses memory values cached for a second, assumes non-v8 working set is stable and starts to aggressively GC if the v8 heap grows more then 2 times larger then the non-v8 one... > > So I think it will work in multi-threaded sense, with heuristic causing less aggressive GCs when dedicated workers are present, which is probably ok. > > I wasn't able to find the initial motivation for the specific heuristic, hopefully Anton or Adam can chime in. Created attachment 163687 [details]
Added a mutex in V8GCController
Updated the patch. It now has a mutex around setting/reading the shared workingSetEstimateMB variable.
Comment on attachment 163687 [details] Added a mutex in V8GCController Attachment 163687 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13835326 Comment on attachment 163687 [details]
Added a mutex in V8GCController
Looks good to me (with an android build fix).
Created attachment 163730 [details]
Patch for landing
|