Bug 92993 - [V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer constructed and destructed
Summary: [V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer constructe...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ulan Degenbaev
URL:
Keywords:
: 42912 (view as bug list)
Depends on: 42912 64627
Blocks: 94463
  Show dependency treegraph
 
Reported: 2012-08-02 08:11 PDT by Ulan Degenbaev
Modified: 2012-11-20 12:00 PST (History)
5 users (show)

See Also:


Attachments
Patch (4.68 KB, patch)
2012-08-02 08:16 PDT, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
Patch (12.58 KB, patch)
2012-08-06 07:52 PDT, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
Patch (12.75 KB, patch)
2012-08-07 06:49 PDT, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
Add more tests (20.50 KB, patch)
2012-08-08 05:30 PDT, Ulan Degenbaev
no flags Details | Formatted Diff | Diff
Patch (20.37 KB, patch)
2012-08-20 04:09 PDT, Ulan Degenbaev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ulan Degenbaev 2012-08-02 08:11:54 PDT
[V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer constructed and destructed
Comment 1 Ulan Degenbaev 2012-08-02 08:16:20 PDT
Created attachment 156089 [details]
Patch
Comment 2 Kentaro Hara 2012-08-02 08:29:58 PDT
Comment on attachment 156089 [details]
Patch

LGTM. kbr@ will review the patch.
Comment 3 Kenneth Russell 2012-08-02 10:50:36 PDT
Comment on attachment 156089 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156089&action=review

How well tested is this patch? Did you test the performance impact of this change on various WebGL samples and games (like Mandreel's: http://www.mandreel.com/?page_id=1312 )? Does it cause full GCs to be triggered more often? We don't want to penalize applications that allocate a lot of storage using typed arrays but which rely on scavenges to get their performance.

Did you test what happens if you allocate e.g. a new Float32Array(...)? Does this instrumentation catch the new ArrayBuffer that's created behind the scenes but which isn't exposed to JavaScript?

> Source/WebCore/bindings/v8/custom/V8ArrayBufferCustom.cpp:74
> +    v8::V8::AdjustAmountOfExternalAllocatedMemory(buffer->byteLength());

Add a comment indicating this is balanced in the autogenerated code for V8ArrayBuffer::derefObject.
Comment 4 Kenneth Russell 2012-08-02 11:22:02 PDT
Comment on attachment 156089 [details]
Patch

After offline discussion with Ulan, this patch definitely isn't handling the following cases correctly: (1) allocation of a new view (like Float32Array) which causes allocation of a new ArrayBuffer; and (2) transfer of ownership of an ArrayBuffer.
Comment 5 Ulan Degenbaev 2012-08-06 07:52:04 PDT
Created attachment 156693 [details]
Patch
Comment 6 Ulan Degenbaev 2012-08-06 08:06:04 PDT
I uploaded new patch that handles allocation of new view and transfer of ownership. It does so by adding a callback member to the ArrayBufferContents. This callback is invoked when the buffer is destructed and it calls V8::AdjustAmountOfExternalAllocatedMemory .

The patch is not ready to be landed, I uploaded it to check whether this approach is reasonable or not and to get some feedback.

In particular, I would appreciate suggestions about:
- ownership of the callback member (at the moment I assume that it is a static singleton and don't use any smart pointers).
- wtf/ArrayBuffer.h now looks ugly with all those ifdefs, is there a way to avoid it?

The patch depends on http://code.google.com/p/v8/source/detail?r=12262 

I tested performance with a few html5 benchmarks and didn't notice performance degradation so far. I am going to test more.
Comment 7 Ulan Degenbaev 2012-08-07 06:49:48 PDT
Created attachment 156930 [details]
Patch
Comment 8 Ulan Degenbaev 2012-08-07 06:56:35 PDT
I uploaded new patch that fixes array buffer transfer. Now it decrements the external memory counter of the current isolate. I am looking for place where I could increment the external memory counter for the receiving isolate.

On the V8 side, I am adding debug tracing of the external memory counter and exposing it to privileged JS code so that I can write better tests.
Comment 9 Kenneth Russell 2012-08-08 02:01:14 PDT
Sorry for the delay, but I probably won't be able to review this patch until Friday 8/10.
Comment 10 Ulan Degenbaev 2012-08-08 05:30:01 PDT
Created attachment 157193 [details]
Add more tests
Comment 11 Ulan Degenbaev 2012-08-08 05:33:30 PDT
No problem, Ken. The new patch that I uploaded depends on http://code.google.com/p/v8/source/detail?r=12272 which probably won't be rolled in Chromium until Thursday/Friday anyway.

I added tests that check various ways to construct typed arrays and also check transfer of a buffer to a worker.
Comment 12 Kenneth Russell 2012-08-10 20:10:11 PDT
Comment on attachment 157193 [details]
Add more tests

View in context: https://bugs.webkit.org/attachment.cgi?id=157193&action=review

This looks much better than the last patch. The tests look good and thorough. I'm only r-'ing this because of the license issue on the new header. A couple of other questions.

> Source/WTF/wtf/ArrayBuffer.h:40
> +class ArrayBufferDeallocationObserver {

You might want to add a comment that in the current implementation, the instance of this must be a singleton living for the entire process's lifetime (implicit because you're using a raw pointer to it in ArrayBuffer -- I'm not suggesting to change that because of performance considerations).

> Source/WTF/wtf/ArrayBuffer.h:88
> +        // Notify the current V8 isolate that the buffer is gone.

Are you sure this is the behavior you want to implement? Conceptually, the other isolate should increase the amount of externally allocated memory, and the deallocation observer should be copied to the newly allocated ArrayBuffer. I realize this would involve more code, but do you at least want to add a FIXME here?

> Source/WebCore/bindings/v8/custom/V8ArrayBufferCustom.h:2
> + * Copyright (C) 2012 Google Inc. All rights reserved.

Wrong license. Use the two-clause license in Source/WebKit/LICENSE.
Comment 13 Ulan Degenbaev 2012-08-20 04:09:03 PDT
Created attachment 159389 [details]
Patch
Comment 14 Ulan Degenbaev 2012-08-20 04:11:56 PDT
Comment on attachment 157193 [details]
Add more tests

View in context: https://bugs.webkit.org/attachment.cgi?id=157193&action=review

Thank you! I uploaded a new rebased patch.

>> Source/WTF/wtf/ArrayBuffer.h:40
>> +class ArrayBufferDeallocationObserver {
> 
> You might want to add a comment that in the current implementation, the instance of this must be a singleton living for the entire process's lifetime (implicit because you're using a raw pointer to it in ArrayBuffer -- I'm not suggesting to change that because of performance considerations).

Done.

>> Source/WTF/wtf/ArrayBuffer.h:88
>> +        // Notify the current V8 isolate that the buffer is gone.
> 
> Are you sure this is the behavior you want to implement? Conceptually, the other isolate should increase the amount of externally allocated memory, and the deallocation observer should be copied to the newly allocated ArrayBuffer. I realize this would involve more code, but do you at least want to add a FIXME here?

I opened a new issue (bug 94463) to track this. I think it should be possible to increase the amount of externally allocated memory and set the deallocation observer on the receiving isolate during deserialization when we create a V8 wrapper for the typed array.

>> Source/WebCore/bindings/v8/custom/V8ArrayBufferCustom.h:2
>> + * Copyright (C) 2012 Google Inc. All rights reserved.
> 
> Wrong license. Use the two-clause license in Source/WebKit/LICENSE.

Done.
Comment 15 Kenneth Russell 2012-08-20 13:22:29 PDT
Comment on attachment 159389 [details]
Patch

Thanks, looks good. r=me
Comment 16 WebKit Review Bot 2012-08-21 15:32:09 PDT
Comment on attachment 159389 [details]
Patch

Clearing flags on attachment: 159389

Committed r126196: <http://trac.webkit.org/changeset/126196>
Comment 17 WebKit Review Bot 2012-08-21 15:32:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 Kenneth Russell 2012-11-20 12:00:02 PST
*** Bug 42912 has been marked as a duplicate of this bug. ***