Bug 42912 – Consider using v8::AdjustAmountOfExternalAllocatedMemory for TypedArrays

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebGL

[Kenneth Russell]

We should consider calling v8::AdjustAmountOfExternalAllocatedMemory when TypedArrays and ArrayBuffers are allocated and deallocated. However, we need to be cautious about this because doing so may provoke full GCs, which would be bad for applications. Need to ask the V8 team for guidance.

[James Robinson]

I think we should do this.  We recently received a report from a chromium extension developer who was loading the data from a series of files via XHR and accessing the data as an ArrayBuffer via responseType="arraybuffer".  Even though only one ArrayBuffer is reachable through javascript at any given time, the page quickly exhausts available RAM since there aren't enough allocations within javascript memory to trigger a GC.

[James Robinson]

Created attachment 99508 [details]
Patch

[WebKit Review Bot]

Comment on attachment 99508 [details]
Patch

Attachment 99508 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8974266

New failing tests:
fast/canvas/webgl/array-buffer-memory.html

[WebKit Review Bot]

Created attachment 99515 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[James Robinson]

Looks like the test timed out, which seems possible.  I'm not sure how to construct a test case that doesn't allocate a ton of memory, which is going to be slow, since otherwise this patch should not change behavior.

[James Robinson]

Created attachment 100028 [details]
Patch

[Kenneth Russell]

Comment on attachment 100028 [details]
Patch

Looks good. Thanks for picking this up.

[WebKit Review Bot]

Comment on attachment 100028 [details]
Patch

Rejecting attachment 100028 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-03', '--port..." exit_code: 2

Last 500 characters of output:
b4983f42a5417e5da321b861357482a455302b42
r90590 = 84ffd71402ca5049c49680dba1edd66b0d61685c
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Updating chromium port dependencies using gclient...

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/8995536

[WebKit Review Bot]

Comment on attachment 100028 [details]
Patch

Clearing flags on attachment: 100028

Committed r90592: <http://trac.webkit.org/changeset/90592>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Robinson]

Uh oh, there's an issue (discovered via crash reports from users): we allocate ArrayBuffers from several threads, but v8::V8::AdjustAmountOfExternalAllocatedMemory() is only safe to call from the main thread.

I only want to run this for ArrayBuffers that are being directly referenced from script, so perhaps I need to move the call closer to the bindings level.  Will investigate.

[Kenneth Russell]

We should consider filing another bug, unless you want to revert the original patch.

I'd suggest passing some enum flag from the bindings to the ArrayBuffer create() methods which indicates whether the JavaScript engine should be notified about the allocation. It would be best to keep the tracking in shared code as you currently have it.

[James Robinson]

I think I'll revert, this approach is slightly wrong :(.  In the web audio asynchronous decoding API, for example, the ArrayBuffer is allocated on a background thread and then passed to the calling thread to be wrapped.  At the time the ArrayBuffer is allocated there is no way to know which heap to associate the memory usage with since the caller could be a the main context, a worker context, or an isolated world in the main thread or a worker.  I think the AdjustAmountOf...() call needs to be made closer to the place where we create the javascript wrapper so we can associate it with the correct heap.

[anton muhin]

Just FYI: we have additional safe guards which should solve the issue with huge natively allocated objects retained by JS.  We just check that if renderer process consumes too much of memory, than it might be worth forcing another GC.

I think approach above should be preferred over AdjustEx... stuff.

(In reply to comment #1)
> I think we should do this.  We recently received a report from a chromium extension developer who was loading the data from a series of files via XHR and accessing the data as an ArrayBuffer via responseType="arraybuffer".  Even though only one ArrayBuffer is reachable through javascript at any given time, the page quickly exhausts available RAM since there aren't enough allocations within javascript memory to trigger a GC.

[James Robinson]

Those guards are really generous and don't actually help users too much.  They are also impossible to correlate directly with a specific heap, which means once workers + isolated worlds land that path will be even less useful.

[anton muhin]

(In reply to comment #15)
> Those guards are really generous and don't actually help users too much.  They are also impossible to correlate directly with a specific heap, which means once workers + isolated worlds land that path will be even less useful.

Trust me, they do help, otherwise they won't be added :)

Workers in the same process is a good point, but that would just require going over all the heaps, so I don't think it'd be that difficult to implement.

Using ::Adjust has hard to predict performance implications.

[Eric Seidel (no email)]

Yeah, I'm surprised we wouldn't do this from the bindings layer, since I would expect we'd want the reporting to be per-isolate.  I suspect we have a bunch of similar bugs of under-reporting of external memory for v8.

[Kenneth Russell]

(In reply to comment #17)
> Yeah, I'm surprised we wouldn't do this from the bindings layer, since I would expect we'd want the reporting to be per-isolate.  I suspect we have a bunch of similar bugs of under-reporting of external memory for v8.

Since this bug was originally filed, v8::AdjustAmountOfExternalAllocatedMemory has been made thread-safe, and Ulan Degenbaev has added calls to it for ArrayBuffer allocation and deallocation, even handling the case of Transferable typed arrays:

https://bugs.webkit.org/show_bug.cgi?id=92993
https://bugs.webkit.org/show_bug.cgi?id=94463

I'm proactively assigning this bug to Ulan and closing it as a duplicate of Issue 92993.

*** This bug has been marked as a duplicate of bug 92993 ***