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().
Created attachment 187041 [details] Patch
Created attachment 187042 [details] Patch
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?
(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?
kbr: would you mind taking a look at the patch?
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.
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.)
(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?
(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.
(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.
(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?
(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.
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.
(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?
> 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.
V8 is gone from WebKit.