Fixing laggy chrome on a multitude of accelerated canvases

third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp b/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
index e04ea2b304f96f0bc3e3c80e013ff43918bbac47..a1ca43ac74803667a4fe19914b9fd0fa9467534d 100644
--- a/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp
@@ -80,6 +80,10 @@ const int MaxCanvasArea = 32768 * 8192; // Maximum canvas area in CSS pixels
 // In Skia, we will also limit width/height to 32767.
 const int MaxSkiaDim = 32767; // Maximum width/height in CSS pixels.
 
+// Max Limit of Externally Allocated Memory

[danakj, 2015/11/30 23:07:03]

comments are sentences, please add periods, and don't need to capitalize
everything like a title.

+// Equivalent to memory used by 80 accelerated canvases, each has a size of 1000*500 and 2d context
+const int MaxTotalExternallyAllocatedMemory = 6000000 * 80; 

[danakj, 2015/11/30 23:07:03]

also, naming for contants is kNameOfConstant. but then, the other consts in this
file are wrong too so idk.

[danakj, 2015/11/30 23:07:03]

can you write it as like 1000*500*4.. i'm not sure what the extra *3 would be.
Maybe the comment can explain the other factors in this number too.

[xlai (Olivia), 2015/12/01 21:50:25]

Refactored all the constants in this file to fit the naming convention.

+
 // A default value of quality argument for toDataURL and toBlob
 // It is in an invalid range (outside 0.0 - 1.0) so that it will not be misinterpreted as a user-input value
 const int UndefinedQualityValue = -1.0;
@@ -124,9 +128,13 @@ inline HTMLCanvasElement::HTMLCanvasElement(Document& document)
 
 DEFINE_NODE_FACTORY(HTMLCanvasElement)
 
+intptr_t HTMLCanvasElement::s_totalMemoryForAcceleratedCanvases = 0;
+
 HTMLCanvasElement::~HTMLCanvasElement()
 {
     v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(-m_externallyAllocatedMemory);
+    if (m_imageBuffer && m_imageBuffer->isAccelerated())
+        updateTotalMemoryForAcceleratedCanvases(-m_externallyAllocatedMemory);
 #if !ENABLE(OILPAN)
     // Ensure these go away before the ImageBuffer.
     m_context.clear();
@@ -605,7 +613,7 @@ bool HTMLCanvasElement::shouldAccelerate(const IntSize& size) const
             return false;
 #endif
         // If the GPU resources would be very expensive, prefer a display list.
-        if (canvasPixelCount > ExpensiveCanvasHeuristicParameters::PreferDisplayListOverGpuSizeThreshold)
+        if (canvasPixelCount >= ExpensiveCanvasHeuristicParameters::PreferDisplayListOverGpuSizeThreshold)

[danakj, 2015/11/30 23:07:03]

This looks like a separate CL, cuz it's an unrelated change to what this CL
means to do?

[xlai (Olivia), 2015/12/01 21:50:25]

Sorry that's a careless mistake. Had corrected it.

             return false;
     }
 
@@ -616,6 +624,19 @@ bool HTMLCanvasElement::shouldAccelerate(const IntSize& size) const
     if (!Platform::current()->canAccelerate2dCanvas())
         return false;
 
+    // When GPU allocated memory runs low (due to having created too many accelerated canvases), the
+    //   compositor starves and browser becomes laggy. Thus, we should stop allocating more GPU memory to 

[danakj, 2015/11/30 23:07:03]

don't indent comments extra, all lines get the same whitespace.

+    //   new canvases created when the current memory usage exceeds the threshold.
+    if (s_totalMemoryForAcceleratedCanvases >= MaxTotalExternallyAllocatedMemory)

[danakj, 2015/11/30 23:07:03]

You'll go over the budget before you start rejecting. Should you be checking
(totalAllocatedMemory + memoryThisCanvasWillUse) instead?

[xlai (Olivia), 2015/12/01 21:50:25]

I think the max limit here is more like a soft limit, not a hard limit. Creating
one more accelerated canvas over the limit is fine. We just need to prevent
creating lots and lots of accelerated canvases beyond the limit.

Also, this line of code is wrapped in the shouldAccelerate() function that is
invoked when creating a new image buffer for a freshly created canvas. One
important factor in determining the memoryThisCanvasWillUse is whether it is
accelerated. So, implementing an estimate within the shouldAccelerate() is kind
of a "recursive" logic that might be confusing to readers.

+        return false;
+#if defined(OS_ANDROID)

[danakj, 2015/11/30 23:07:03]

I think you can do this ifdefing around what the limit is instead of putting it
in the code here instead.

And maybe this can be a platform setting instead of an #ifdef? Not sure how
blink does this normally.

[xlai (Olivia), 2015/12/01 21:50:25]

Acknowledged. Moved the code to the front in setting the max. Also, changed to
OS(ANDROID) to detect platform because this is used in other codes in
third_party/WebKit.

+    // We estimate that the max limit for android phones is half of that for desktops 
+    //   based on local experimental results on Android One
+    // (though a very slight lagginess on Android One is still expected, other devices should work fine)
+    if (s_totalMemoryForAcceleratedCanvases >= MaxTotalExternallyAllocatedMemory / 2)
+        return false;
+#endif
+
     return true;
 }
 
@@ -776,8 +797,25 @@ void HTMLCanvasElement::updateExternallyAllocatedMemory() const
         externallyAllocatedMemory = std::numeric_limits<intptr_t>::max();
 
     // Subtracting two intptr_t that are known to be positive will never underflow.
-    v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(externallyAllocatedMemory - m_externallyAllocatedMemory);
+    intptr_t diffFromCurrAllocatedMemory = externallyAllocatedMemory - m_externallyAllocatedMemory;
+    v8::Isolate::GetCurrent()->AdjustAmountOfExternalAllocatedMemory(diffFromCurrAllocatedMemory);
     m_externallyAllocatedMemory = externallyAllocatedMemory;
+
+    if (m_imageBuffer && m_imageBuffer->isAccelerated())
+        updateTotalMemoryForAcceleratedCanvases(diffFromCurrAllocatedMemory);
+}
+
+void HTMLCanvasElement::updateTotalMemoryForAcceleratedCanvases(intptr_t diffFromCurrAllocatedMemory)
+{
+    ASSERT(isMainThread());
+
+    Checked<intptr_t, RecordOverflow> checkedExternallyAllocatedMemory = s_totalMemoryForAcceleratedCanvases;
+    checkedExternallyAllocatedMemory += diffFromCurrAllocatedMemory;
+    intptr_t externallyAllocatedMemory;
+    if (checkedExternallyAllocatedMemory.safeGet(externallyAllocatedMemory) == CheckedState::DidOverflow)
+        externallyAllocatedMemory = std::numeric_limits<intptr_t>::max();
+
+    s_totalMemoryForAcceleratedCanvases = externallyAllocatedMemory;
 }
 
 SkCanvas* HTMLCanvasElement::drawingCanvas() const