Bug 109166 – [V8] Call V8::AdjustAmountOfExternalAllocatedMemory() correctly in SerializedScriptValue

• Component: WebCore JavaScript
• Status: RESOLVED WONTFIX

[Kentaro Hara]

Currently V8::AdjustAmountOfExternalAllocatedMemory() is called when a MessageEvent() is created. This results in #if USE(V8) macros in WebCore/dom/MessageEvent.cpp.

Alternately, V8::AdjustAmountOfExternalAllocatedMemory() should be called from v8/SerializedScriptValue.cpp in sync with updating SerializedScriptValue::m_data.

In addition, to estimate the amount of memory used, we should use m_data.sizeInBytes() instead of m_data.length().

[Kentaro Hara]

Created attachment 187041 [details]
Patch

[Kentaro Hara]

Created attachment 187042 [details]
Patch

[Adam Barth]

Comment on attachment 187042 [details]
Patch

This patch seems reasonable to me, but I don't know much about AdjustAmountOfExternalAllocatedMemory.  Who's the best person to review this change?

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 187042 [details])
> Who's the best person to review this change?

kbr: Would you take a look?

[Kentaro Hara]

kbr: would you mind taking a look at the patch?

[Kenneth Russell]

Comment on attachment 187042 [details]
Patch

Sorry for the delay reviewing this. I must be missing something because neither the previous code nor the new code look correct. I probably r+'d the earlier bad patch and apologize for that. It looks like m_data is a string representing the serialized object graph, but the whole point of transferring ArrayBuffers is that their contents aren't serialized -- they're transferred from one isolate to another. The amount of external allocated memory needs to be the sum of the transferred ArrayBuffers' sizes, not the size of the m_data string. I'm not sure exactly how and where this should be computed.

[Kentaro Hara]

Thanks kbr!

> The amount of external allocated memory needs to be the sum of the transferred ArrayBuffers' sizes

I guess that we can calculate that by calling AdjustMemory() in sync with updating m_data.

(0) Assume that a buffer is transferred from A to B.
(1) SerializedScriptValue is allocated in A. AdjustMemory(+sizeof(m_data)) is called in A.
(2) The buffer is transferred to B.
(3) SerializedScriptValue is allocated in B. AdjustMemory(+sizeof(m_data)) is called in B.
(4) SerializedScriptValue is destructed in A. AdjustMemory(-sizeof(m_data)) is called in A. (This happens in the destructor of SerializedScriptValue.)

[Kenneth Russell]

(In reply to comment #7)
> Thanks kbr!
> 
> > The amount of external allocated memory needs to be the sum of the transferred ArrayBuffers' sizes
> 
> I guess that we can calculate that by calling AdjustMemory() in sync with updating m_data.
> 
> (0) Assume that a buffer is transferred from A to B.
> (1) SerializedScriptValue is allocated in A. AdjustMemory(+sizeof(m_data)) is called in A.
> (2) The buffer is transferred to B.
> (3) SerializedScriptValue is allocated in B. AdjustMemory(+sizeof(m_data)) is called in B.
> (4) SerializedScriptValue is destructed in A. AdjustMemory(-sizeof(m_data)) is called in A. (This happens in the destructor of SerializedScriptValue.)

To be clear, it isn't sizeof(m_data), it's sizeOfTransferredArrayBuffers, right?

Won't the SerializedScriptValue in isolate B be destroyed soon afterward and then AdjustMemory(-sizeOfTransferredArrayBuffers) will be called in B?

[Kentaro Hara]

(In reply to comment #8)
> To be clear, it isn't sizeof(m_data), it's sizeOfTransferredArrayBuffers, right?

Yes.

> Won't the SerializedScriptValue in isolate B be destroyed soon afterward and then AdjustMemory(-sizeOfTransferredArrayBuffers) will be called in B?

It depends on lifetime of a SerializedScriptValue. If someone keeps a reference to a wrapper of a SerializedScriptValue, then the transferred buffer is kept alive, being represented as m_data of the SerializedScriptValue. My point is that live transferred buffer is represented by some SerializedScriptValue, and thus we can calculate the amount of memory by watching the size of m_data of all SerializedScriptValues.

[Kenneth Russell]

(In reply to comment #9)
> (In reply to comment #8)
> > To be clear, it isn't sizeof(m_data), it's sizeOfTransferredArrayBuffers, right?
> 
> Yes.
> 
> > Won't the SerializedScriptValue in isolate B be destroyed soon afterward and then AdjustMemory(-sizeOfTransferredArrayBuffers) will be called in B?
> 
> It depends on lifetime of a SerializedScriptValue. If someone keeps a reference to a wrapper of a SerializedScriptValue, then the transferred buffer is kept alive, being represented as m_data of the SerializedScriptValue. My point is that live transferred buffer is represented by some SerializedScriptValue, and thus we can calculate the amount of memory by watching the size of m_data of all SerializedScriptValues.

Really, though, the amount of external allocated memory is tied to the ArrayBuffer instance, not the SerializedScriptValue which helped transfer it from one isolate to another.

[Kentaro Hara]

(In reply to comment #10)
> Really, though, the amount of external allocated memory is tied to the ArrayBuffer instance, not the SerializedScriptValue which helped transfer it from one isolate to another.

Thanks, getting clearer to me...

- SerializedScriptValue is used just for a temporary store to transfer a buffer. So we don't need to watch m_data's size. (If it's not temporal, it is worth watching m_data's size, but that's not true.)

- Alternately, we need to watch ArrayBuffer's size. We need to insert AdjustMemory() to the points where ArrayBuffer's size is updated.

Is my understanding correct?

[Kenneth Russell]

(In reply to comment #11)
> (In reply to comment #10)
> > Really, though, the amount of external allocated memory is tied to the ArrayBuffer instance, not the SerializedScriptValue which helped transfer it from one isolate to another.
> 
> Thanks, getting clearer to me...
> 
> - SerializedScriptValue is used just for a temporary store to transfer a buffer. So we don't need to watch m_data's size. (If it's not temporal, it is worth watching m_data's size, but that's not true.)
> 
> - Alternately, we need to watch ArrayBuffer's size. We need to insert AdjustMemory() to the points where ArrayBuffer's size is updated.
> 
> Is my understanding correct?

I think so but I haven't studied how SerializedScriptValue is allocated and used during postMessage and afterward. I wish that someone who worked on this code could provide input. CC'ing a couple more people.

[Ulan Degenbaev]

I worked on https://bugs.webkit.org/show_bug.cgi?id=94463 and https://bugs.webkit.org/show_bug.cgi?id=92993 that call AdjustAmountOfExternalAllocatedMemory() when ArrayBuffers are transferred. Before these changes, transferring ArrayBuffer could cause OOM.

[Kentaro Hara]

(In reply to comment #13)
> I worked on https://bugs.webkit.org/show_bug.cgi?id=94463 and https://bugs.webkit.org/show_bug.cgi?id=92993 that call AdjustAmountOfExternalAllocatedMemory() when ArrayBuffers are transferred. Before these changes, transferring ArrayBuffer could cause OOM.

Does it mean we don't need to call (or we shouldn't call) AdjustMemory() when a MessageEvent is created?

[Ulan Degenbaev]

> Does it mean we don't need to call (or we shouldn't call) AdjustMemory() when a MessageEvent is created?

I think we don't need it here for array buffers, but there might be other object types that need it.

[Anders Carlsson]

V8 is gone from WebKit.