There are repeated cases when Workers processes crash in Chromium due to OOM. Here is an example: http://code.google.com/p/chromium/issues/detail?id=145982 This happens because we don't use the same technique that we use on the main thread for measuring the overall amount of memory allocated and hint the V8 GC to collect garbage in case it goes over certain threshold. Often quite a big memory areas are hanging on a relatively small V8 objects which V8 GC is not in a hurry to collect. For main thread, it was fixed (bug 31051), we need to do the same for Workers. Patch is coming.
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
Landed: http://trac.webkit.org/changeset/128390