Bug 42912 - Consider using v8::AdjustAmountOfExternalAllocatedMemory for TypedArrays
: Consider using v8::AdjustAmountOfExternalAllocatedMemory for TypedArrays
Status: RESOLVED DUPLICATE of bug 92993
: WebKit
WebGL
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To:
:
:
: 64627
: 92993
  Show dependency treegraph
 
Reported: 2010-07-23 15:02 PST by
Modified: 2012-11-20 12:00 PST (History)


Attachments
Patch (4.55 KB, patch)
2011-07-01 12:47 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (1.46 MB, application/zip)
2011-07-01 13:36 PST, WebKit Review Bot
no flags Details
Patch (2.94 KB, patch)
2011-07-07 13:56 PST, James Robinson
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-07-23 15:02:00 PST
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.
------- Comment #1 From 2011-06-30 16:42:32 PST -------
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.
------- Comment #2 From 2011-07-01 12:47:28 PST -------
Created an attachment (id=99508) [details]
Patch
------- Comment #3 From 2011-07-01 13:36:03 PST -------
(From update of attachment 99508 [details])
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
------- Comment #4 From 2011-07-01 13:36:08 PST -------
Created an attachment (id=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
------- Comment #5 From 2011-07-01 14:17:43 PST -------
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.
------- Comment #6 From 2011-07-07 13:56:39 PST -------
Created an attachment (id=100028) [details]
Patch
------- Comment #7 From 2011-07-07 14:10:59 PST -------
(From update of attachment 100028 [details])
Looks good. Thanks for picking this up.
------- Comment #8 From 2011-07-07 14:36:23 PST -------
(From update of attachment 100028 [details])
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
------- Comment #9 From 2011-07-07 15:09:49 PST -------
(From update of attachment 100028 [details])
Clearing flags on attachment: 100028

Committed r90592: <http://trac.webkit.org/changeset/90592>
------- Comment #10 From 2011-07-07 15:09:56 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #11 From 2011-07-14 17:20:21 PST -------
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.
------- Comment #12 From 2011-07-14 18:00:53 PST -------
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.
------- Comment #13 From 2011-07-15 14:16:33 PST -------
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.
------- Comment #14 From 2011-07-18 12:19:18 PST -------
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.
------- Comment #15 From 2011-07-18 12:23:36 PST -------
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.
------- Comment #16 From 2011-07-18 12:27:54 PST -------
(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.
------- Comment #17 From 2012-11-20 02:13:53 PST -------
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.
------- Comment #18 From 2012-11-20 12:00:02 PST -------
(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 ***