Bug 96459 – [V8] OOM in Workers due to external memory retention.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Dmitry Titov]

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.

[Dmitry Titov]

Created attachment 163500 [details]
Proposed patch.

[David Levin]

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.

[Dmitry Titov]

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.

[anton muhin]

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.

[Dmitry Titov]

Created attachment 163687 [details]
Added a mutex in V8GCController

Updated the patch. It now has a mutex around setting/reading the shared workingSetEstimateMB variable.

[Peter Beverloo (cr-android ews)]

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

[David Levin]

Comment on attachment 163687 [details]
Added a mutex in V8GCController

Looks good to me (with an android build fix).

[Dmitry Titov]

Created attachment 163730 [details]
Patch for landing

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/128390