|Summary:||Consider using v8::AdjustAmountOfExternalAllocatedMemory for TypedArrays|
|Product:||WebKit||Reporter:||Kenneth Russell <kbr>|
|Component:||WebGL||Assignee:||Ulan Degenbaev <ulan>|
|Severity:||Normal||CC:||abarth, antonm, arv, crogers, danno, dglazkov, enne, eric, jamesr, jochen, ulan, webkit.review.bot, zmo|
|Version:||528+ (Nightly build)|
|Bug Depends on:||64627|
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
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 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
Comment 13 James Robinson 2011-07-15 14:16:33 PDT
Comment 14 anton muhin 2011-07-18 12:19:18 PDT
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 (no email) 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 ***