Bug 170690

Summary: WebAssembly: report Memory usage to GC
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, jfbastien, keith_miller, mark.lam, msaboff, saam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 170628    
Bug Blocks: 159775    
Attachments:
Description Flags
patch none

Description JF Bastien 2017-04-10 13:42:42 PDT
As part of bug #170628 I noticed that we never report the memory allocated by WebAssembly.Memory to the GC. This change would do it:

Wasm::PageCount JSWebAssemblyMemory::grow(VM& vm, ExecState* exec, uint32_t delt
...
    vm.heap.reportExtraMemoryAllocated(Wasm::PageCount(delta).bytes());

void JSWebAssemblyMemory::finishCreation(VM& vm)
...
    heap()->reportExtraMemoryAllocated(m_memory->size());

JSWebAssemblyMemory::visitChildren(JSCell* cell, SlotVisitor& visitor)
...
    visitor.reportExtraMemoryVisited(thisObject->memory().size());


I'm not sure it's the right thing to do though! The "initial" size seems like a good thing to report, but if we haven't actually backed it up with physical pages then it may severely over-estimate allocation and cause the GC to run too much.

This change is significant enough that I'll punt it to its own patch, after bug #170628.
Comment 1 Radar WebKit Bug Importer 2017-05-03 09:54:31 PDT
<rdar://problem/31965310>
Comment 2 JF Bastien 2017-05-11 00:32:32 PDT
Created attachment 309699 [details]
patch

I'm still not sure this is the right thing. We definitely should tell the GC about this memory but:

 1. It's more memory than other allocations. Maybe that's the same as ArrayBuffer and doesn't matter?
 2. We're not sure all pages are dirty, so we could be over-reporting if the developer over-allocates. We just don't know if they do.

I think these don't matter, and we can adjust if they do, so this patch is probably right. I'd nonetheless would rather ask and get feedback on these points.
Comment 3 Keith Miller 2017-05-15 23:56:19 PDT
Comment on attachment 309699 [details]
patch

r=me.
Comment 4 WebKit Commit Bot 2017-05-16 00:09:59 PDT
Comment on attachment 309699 [details]
patch

Clearing flags on attachment: 309699

Committed r216911: <http://trac.webkit.org/changeset/216911>
Comment 5 WebKit Commit Bot 2017-05-16 00:10:02 PDT
All reviewed patches have been landed.  Closing bug.