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

JF Bastien
Reported 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.
Attachments
patch (2.80 KB, patch)
2017-05-11 00:32 PDT, JF Bastien 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2017-05-03 09:54:31 PDT
<rdar://problem/31965310>
JF Bastien
Comment 2 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.
Keith Miller
Comment 3 2017-05-15 23:56:19 PDT
Comment on attachment 309699 [details] patch r=me.
WebKit Commit Bot
Comment 4 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>
WebKit Commit Bot
Comment 5 2017-05-16 00:10:02 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.