Bug 92993 – [V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer constructed and destructed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Ulan Degenbaev]

[V8] Call AdjustAmountOfExternalAllocatedMemory when V8ArrayBuffer constructed and destructed

[Ulan Degenbaev]

Created attachment 156089 [details]
Patch

[Kentaro Hara]

Comment on attachment 156089 [details]
Patch

LGTM. kbr@ will review the patch.

[Kenneth Russell]

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.

[Kenneth Russell]

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.

[Ulan Degenbaev]

Created attachment 156693 [details]
Patch

[Ulan Degenbaev]

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.

[Ulan Degenbaev]

Created attachment 156930 [details]
Patch

[Ulan Degenbaev]

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.

[Kenneth Russell]

Sorry for the delay, but I probably won't be able to review this patch until Friday 8/10.

[Ulan Degenbaev]

Created attachment 157193 [details]
Add more tests

[Ulan Degenbaev]

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.

[Kenneth Russell]

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.

[Ulan Degenbaev]

Created attachment 159389 [details]
Patch

[Ulan Degenbaev]

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.

[Kenneth Russell]

Comment on attachment 159389 [details]
Patch

Thanks, looks good. r=me

[WebKit Review Bot]

Comment on attachment 159389 [details]
Patch

Clearing flags on attachment: 159389

Committed r126196: <http://trac.webkit.org/changeset/126196>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

*** Bug 42912 has been marked as a duplicate of this bug. ***