Bug 42912 - Consider using v8::AdjustAmountOfExternalAllocatedMemory for TypedArrays
: Consider using v8::AdjustAmountOfExternalAllocatedMemory for TypedArrays
Status: RESOLVED DUPLICATE of bug 92993
Product: WebKit
Classification: Unclassified
Component: WebGL
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To: Ulan Degenbaev
:
Depends on: 64627
Blocks: 92993
  Show dependency treegraph
 
Reported: 2010-07-23 15:02 PDT by Kenneth Russell
Modified: 2012-11-20 12:00 PST (History)
13 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Russell 2010-07-23 15:02:00 PDT
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 James Robinson 2011-06-30 16:42:32 PDT
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 James Robinson 2011-07-01 12:47:28 PDT
Created attachment 99508 [details]
Patch
Comment 3 WebKit Review Bot 2011-07-01 13:36:03 PDT
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
Comment 4 WebKit Review Bot 2011-07-01 13:36:08 PDT
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
Comment 5 James Robinson 2011-07-01 14:17:43 PDT
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 James Robinson 2011-07-07 13:56:39 PDT
Created attachment 100028 [details]
Patch
Comment 7 Kenneth Russell 2011-07-07 14:10:59 PDT
Comment on attachment 100028 [details]
Patch

Looks good. Thanks for picking this up.
Comment 8 WebKit Review Bot 2011-07-07 14:36:23 PDT
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
Comment 9 WebKit Review Bot 2011-07-07 15:09:49 PDT
Comment on attachment 100028 [details]
Patch

Clearing flags on attachment: 100028

Committed r90592: <http://trac.webkit.org/changeset/90592>
Comment 10 WebKit Review Bot 2011-07-07 15:09:56 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 James Robinson 2011-07-14 17:20:21 PDT
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 Kenneth Russell 2011-07-14 18:00:53 PDT
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 James Robinson 2011-07-15 14:16:33 PDT
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 anton muhin 2011-07-18 12:19:18 PDT
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 James Robinson 2011-07-18 12:23:36 PDT
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 anton muhin 2011-07-18 12:27:54 PDT
(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 Eric Seidel 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 Kenneth Russell 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 ***