* SUMMARY Memory Timeline shows impossible value for bmalloc size * STEPS TO REPRODUCE 1. Show Resource Usage Overlay on <https://www.apple.com/watch/> => bmalloc size is 17592186044627.1 MB * NOTES - The bmalloc size underflows when we attempt to subtract GC sections from it: > size_t currentGCHeapCapacity = vm->heap.blockBytesAllocated(); > size_t currentGCOwned = vm->heap.extraMemorySize(); > > // Subtract known subchunks from the bmalloc bucket. > // FIXME: Handle running with bmalloc disabled. > data.categories[MemoryCategory::bmalloc].dirtySize -= currentGCHeapCapacity + currentGCOwned; Values on my MacBookPro were: - bmalloc (before) 59076608 bytes - currentGCHeapCapacity 13130752 bytes - currentGCOwned 1097250396 bytes Either the Heap's values have runaway and are larger then they should be, or they are counting memory that isn't entirely in bmalloc zones and so the subtraction is not safe.
With some logging it doesn't seem like we are duplicate counting any values. The biggest values were coming from: > void JSHTMLCanvasElement::visitChildren(JSCell* cell, SlotVisitor& visitor) > { > auto* thisObject = jsCast<JSHTMLCanvasElement*>(cell); > ASSERT_GC_OBJECT_INHERITS(thisObject, info()); > Base::visitChildren(thisObject, visitor); > thisObject->wrapped().visitJSEventListeners(visitor); > visitor.reportExtraMemoryVisited(thisObject->wrapped().memoryCost()); > } which has: > size_t HTMLCanvasElement::memoryCost() const > { > if (!m_imageBuffer) > return 0; > return 4 * m_imageBuffer->internalSize().width() * m_imageBuffer->internalSize().height(); > } It looks like WebCore::ImageBuffer may be using IOSurface memory instead of bmalloc memory if USE(IOSURFACE_CANVAS_BACKING_STORE) is enabled and the allocation succeeds in that path. Otherwise it looks like it falls back to bmalloc. > ImageBuffer::ImageBuffer(const FloatSize& size, float resolutionScale, ColorSpace imageColorSpace, RenderingMode renderingMode, bool& success) > : m_logicalSize(size) > , m_resolutionScale(resolutionScale) > { > ... > RetainPtr<CGContextRef> cgContext; > if (accelerateRendering) { > #if USE(IOSURFACE_CANVAS_BACKING_STORE) > FloatSize userBounds = sizeForDestinationSize(FloatSize(width.unsafeGet(), height.unsafeGet())); > m_data.surface = IOSurface::create(m_data.backingStoreSize, IntSize(userBounds), imageColorSpace); > cgContext = m_data.surface->ensurePlatformContext(); > if (cgContext) > CGContextClearRect(cgContext.get(), FloatRect(FloatPoint(), userBounds)); > #endif > ... > if (!accelerateRendering) { > if (!tryFastCalloc(m_data.backingStoreSize.height(), m_data.bytesPerRow.unsafeGet()).getValue(m_data.data)) > return; > ... > } > ... > }
Good find! You're right about the fix too, we should stop subtracting the "GC owned" number from the bmalloc number.
<rdar://problem/26498584>
(In reply to comment #2) > Good find! You're right about the fix too, we should stop subtracting the > "GC owned" number from the bmalloc number. When I stopped subtracting I found a startling result: The GC Owned memory (mostly from HTMLCanvasElement::memoryCost) was greater then the entire footprint of the WebContentProcess! Resource Usage - Total Footprint: 120MB Resource Usage - GC Owned Memory: 267MB A number of tools (footprint, vmmap) match the output I'm seeing in ResourceUsageThread concerning the WebContentProcess (120MB dirty memory, ~7MB reusable/purgeable). The memoryCost we associate with the HTMLCanvasElement/ImageBuffer doesn't seem to live in the WebContentProcess when we are using IOSurfaces. I haven't been able to precisely track down where that memory lives, it seems like it is in the kernel, but I'm not sure how to properly measure that. A few tools (IOSDebug, IOAccelMemory) show the surfaces and memory for the particular WebContentProcess. In total the sizes roughly matches the size of GC Owned memory that WebCore is claiming (260MB) but when I dig deeper I only find info about 137MB of allocations, and I'm not sure I even understand what these tools are telling me. Another possibility is that the accounting of the IOSurface memory resides somewhere in the "Virtual" IOKit memory: > VIRTUAL RESIDENT DIRTY SWAPPED VOLATILE NONVOL EMPTY REGION > REGION TYPE SIZE SIZE SIZE SIZE SIZE SIZE SIZE COUNT (non-coalesced) > =========== ======= ======== ===== ======= ======== ====== ===== ======= > ... > IOKit 371.6M 11.9M 11.9M 0K 0K 8K 6372K 272 > ... > TOTAL, minus reserved VM space 1.7G 373.7M 121.1M 84K 0K 3068K 6384K 1513 Ultimately I'm settling on the idea that the actual IOSurface allocations are not in the WebContentProcess. ------- Given that, here are my ideas to fix this bug: Approach 1. Resource Usage should show dirty resident memory for the just the WebContentProcess - the IOSurface memory doesn't appear to contribute to that, so have HTMLCanvasElement::memoryCost() reflect that when it is using an IOSurface which may mean just reporting 0 cost. Approach 2. Resource Usage should show the "total memory cost" incurred by the WebContentProcess, even if it is stored elsewhere. - we could track the IOSurface memory separately, and contribute it to MemoryCategory::Layer memory. I'm siding with Approach 1, because that would mean our numbers match existing tools (vmmap, footprint). Also, if Approach 2 was holistic, it would mean finding other places WebKit causes a lot of memory to show up in other processes. Either way, for a page like this (large canvases), it will mean dramatically different "extra memory cost" values will be reported to VM::Heap. The values will be much more realistic values concerning the memory within the current process. From what I can tell, it may have an affect on GC scheduling.
(In reply to comment #4) > > Another possibility is that the accounting of the IOSurface memory resides > somewhere in the "Virtual" IOKit memory: That would make sense. > Given that, here are my ideas to fix this bug: > > Approach 1. Resource Usage should show dirty resident memory for the just > the WebContentProcess > - the IOSurface memory doesn't appear to contribute to that, so have > HTMLCanvasElement::memoryCost() reflect that when it is using an > IOSurface > which may mean just reporting 0 cost. That seems like a surprising plan if the point of your tool is to indicate the cost of a page to the system. Between canvas and the tiles/compositing layers (don't forget about them!), IOSurface memory is likely a very significant portion - if not a majority - of the memory allocated on behalf of a page.
Created attachment 279940 [details] [PATCH] Proposed Fix
Comment on attachment 279940 [details] [PATCH] Proposed Fix This doesn't make any sense. Of course the memory is "in this process"; this is the process that allocated it, this is the process which you would kill to reclaim it, and this is the process that footprint will blame for it. It's true, it doesn't make any sense to subtract this memory from anything bmalloc-related, but just claiming that IOSurface-backed surfaces are free is crazypants :) Maybe memoryCost() needs to return multiple values?
(In reply to comment #7) > Comment on attachment 279940 [details] > [PATCH] Proposed Fix > > This doesn't make any sense. Of course the memory is "in this process"; this > is the process that allocated it, this is the process which you would kill > to reclaim it, and this is the process that footprint will blame for it. > It's true, it doesn't make any sense to subtract this memory from anything > bmalloc-related, but just claiming that IOSurface-backed surfaces are free > is crazypants :) Maybe memoryCost() needs to return multiple values? I considered making multiple functions with different names. It seems the current trend is WebCore's "memoryCost()" method is "extra memory cost reported to the JavaScript Heap", so I kept that name. I'm willing to rename that.
(In reply to comment #4) > From what I can tell, it may have an affect on GC scheduling. Also, it seems like the GC totally *should* take into account the fact that this fairly small object that it knows about (the canvas element) is potentially the only thing hanging on to an enormous surface. I would like to hear what ggaren has to say about all this before you land it, if possible.
(In reply to comment #9) > (In reply to comment #4) > > From what I can tell, it may have an affect on GC scheduling. > > Also, it seems like the GC totally *should* take into account the fact that > this fairly small object that it knows about (the canvas element) is > potentially the only thing hanging on to an enormous surface. This is a pretty compelling argument to me. I'll look into implementing the alternative approach tomorrow. The alt approach would be basically keeping memory cost reporting things the way they are but keeping an estimate of additional memory that is outside the WebContentProcess (IOSurfaces here) to provide a more accurate total (values should add up!). GC Owned would not all fall under bmalloc so that would also need some tweaking. With bug 158144 implemented, this would mean the <canvas> elements on this page each show up with very high memory use in Web Inspector's JavaScript Allocations view, which is what you would expect if the canvas is the only thing holding onto the larger IOSurface memory.
Comment on attachment 279940 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=279940&action=review > Source/WebCore/ChangeLog:17 > + This patch avoids reporting any canvas IOSurface memory as extra memory I am concerned about this. We need these canvas elements to seem to have cost so that garbage collection will run sooner than it otherwise would. The logic that the cost for memory outside the web process does not matter may be correct for debugging tools or other uses, but it is incorrect for deciding whether it’s important to free the memory in a timely fashion. If we don’t do this we could have a lot of canvases with a lot of big IOSurface objects and garbage collection wouldn’t know that it needs to run to free system memory. > Source/WebCore/ChangeLog:19 > + Process, so incorportating it into Resource Usage, or the Heap to affect Typo: "incorportating"
I see that other people have said the same thing I am saying.
Created attachment 281519 [details] [PATCH] Proposed Fix
Comment on attachment 281519 [details] [PATCH] Proposed Fix r=me though needs some build fixes
Created attachment 281923 [details] [PATCH] For Bots
Comment on attachment 281923 [details] [PATCH] For Bots Clearing flags on attachment: 281923 Committed r202394: <http://trac.webkit.org/changeset/202394>