Bug 158110 – Web Inspector: Memory Timeline sometimes shows impossible value for bmalloc size (underflowed)

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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.

[Joseph Pecoraro]

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;
> ...
>     }
> ...
> }

[Andreas Kling]

Good find! You're right about the fix too, we should stop subtracting the "GC owned" number from the bmalloc number.

[Radar WebKit Bug Importer]

<rdar://problem/26498584>

[Joseph Pecoraro]

(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.

[Tim Horton]

(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.

[Joseph Pecoraro]

Created attachment 279940 [details]
[PATCH] Proposed Fix

[Tim Horton]

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?

[Joseph Pecoraro]

(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.

[Tim Horton]

(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.

[Joseph Pecoraro]

(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.

[Darin Adler]

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"

[Darin Adler]

I see that other people have said the same thing I am saying.

[Joseph Pecoraro]

Created attachment 281519 [details]
[PATCH] Proposed Fix

[Andreas Kling]

Comment on attachment 281519 [details]
[PATCH] Proposed Fix

r=me though needs some build fixes

[Joseph Pecoraro]

Created attachment 281923 [details]
[PATCH] For Bots

[WebKit Commit Bot]

Comment on attachment 281923 [details]
[PATCH] For Bots

Clearing flags on attachment: 281923

Committed r202394: <http://trac.webkit.org/changeset/202394>