Title: [123756] trunk/Source/WebKit/blackberry
- Revision
- 123756
- Author
- commit-qu...@webkit.org
- Date
- 2012-07-26 09:22:25 -0700 (Thu, 26 Jul 2012)
Log Message
[BlackBerry] Refactor BackingStorePrivate::BackingStorePrivate::clearAndUpdateTileOfNotRenderedRegion() to avoid touching tile frontbuffer
https://bugs.webkit.org/show_bug.cgi?id=92095
Patch by Arvid Nilsson <anils...@rim.com> on 2012-07-26
Reviewed by George Staikos.
PR: 141439
Specifically, we want to avoid changing the rendered region of the
front buffer without proper synchronization.
The method is trying to force checkerboard to appear on screen because
an area was invalidated but the render job was dropped, so the tile
contents are now invalid.
Unfortunately it did this in a way which is not thread safe. Fixed by
making it thread safe, in a way that minimizes memory bandwidth usage.
Instead of using the customary sequence of copy-back, modify and swap,
we send a synchronous message to the compositing thread to avoid the
copy-back step and save memory bandwidth. The trade-off is that the
WebKit thread might wait a little longer for the compositing thread
than it would from a waitForCurrentMessage() call.
The way we synchronize this is rather expensive for the WebKit thread,
and this method is called rather carelessly, so add various early
returns to avoid doing it redundantly.
Internally reviewed by Jakob Petsovits and Adam Treat.
* Api/BackingStore.cpp:
(BlackBerry::WebKit::BackingStorePrivate::indexOfTile):
(BlackBerry::WebKit::BackingStorePrivate::clearAndUpdateTileOfNotRenderedRegion):
* Api/BackingStore_p.h:
(BackingStorePrivate):
Modified Paths
Diff
Modified: trunk/Source/WebKit/blackberry/Api/BackingStore.cpp (123755 => 123756)
--- trunk/Source/WebKit/blackberry/Api/BackingStore.cpp 2012-07-26 16:17:21 UTC (rev 123755)
+++ trunk/Source/WebKit/blackberry/Api/BackingStore.cpp 2012-07-26 16:22:25 UTC (rev 123756)
@@ -929,6 +929,9 @@
const Platform::IntRect& backingStoreRect,
bool update)
{
+ if (tileNotRenderedRegion.isEmpty())
+ return;
+
// Intersect the tile with the not rendered region to get the areas
// of the tile that we need to clear.
IntRectList tileNotRenderedRegionRects = tileNotRenderedRegion.rects();
@@ -941,19 +944,45 @@
// Add it again as a regular render job.
m_renderQueue->addToQueue(RenderQueue::RegularRender, tileNotRenderedRegionRect);
}
+ }
- // Find the origin of this tile.
- Platform::IntPoint origin = originOfTile(index, backingStoreRect);
+ // Find the origin of this tile.
+ Platform::IntPoint origin = originOfTile(index, backingStoreRect);
- // Map to tile coordinates.
- tileNotRenderedRegionRect.move(-origin.x(), -origin.y());
+ // Map to tile coordinates.
+ Platform::IntRectRegion translatedRegion(tileNotRenderedRegion);
+ translatedRegion.move(-origin.x(), -origin.y());
- // Clear the tile of this region.
- tile->frontBuffer()->clearRenderedRegion(tileNotRenderedRegionRect);
- tile->backBuffer()->clearRenderedRegion(tileNotRenderedRegionRect);
- }
+ // If the region in question is already marked as not rendered, return early
+ if (Platform::IntRectRegion::intersectRegions(tile->frontBuffer()->renderedRegion(), translatedRegion).isEmpty())
+ return;
+
+ // Clear the tile of this region. The back buffer region is invalid anyway, but the front
+ // buffer must not be manipulated without synchronization with the compositing thread, or
+ // we have a race.
+ // Instead of using the customary sequence of copy-back, modify and swap, we send a synchronous
+ // message to the compositing thread to avoid the copy-back step and save memory bandwidth.
+ // The trade-off is that the WebKit thread might wait a little longer for the compositing thread
+ // than it would from a waitForCurrentMessage() call.
+
+ ASSERT(Platform::webKitThreadMessageClient()->isCurrentThread());
+ if (!Platform::webKitThreadMessageClient()->isCurrentThread())
+ return;
+
+ Platform::userInterfaceThreadMessageClient()->dispatchSyncMessage(
+ Platform::createMethodCallMessage(&BackingStorePrivate::clearRenderedRegion,
+ this, tile, translatedRegion));
}
+void BackingStorePrivate::clearRenderedRegion(BackingStoreTile* tile, const Platform::IntRectRegion& region)
+{
+ ASSERT(Platform::userInterfaceThreadMessageClient()->isCurrentThread());
+ if (!Platform::userInterfaceThreadMessageClient()->isCurrentThread())
+ return;
+
+ tile->frontBuffer()->clearRenderedRegion(region);
+}
+
bool BackingStorePrivate::isCurrentVisibleJob(const TileIndex& index, BackingStoreTile* tile, const Platform::IntRect& backingStoreRect) const
{
// First check if the whole rect is in the queue.
Modified: trunk/Source/WebKit/blackberry/Api/BackingStore_p.h (123755 => 123756)
--- trunk/Source/WebKit/blackberry/Api/BackingStore_p.h 2012-07-26 16:17:21 UTC (rev 123755)
+++ trunk/Source/WebKit/blackberry/Api/BackingStore_p.h 2012-07-26 16:22:25 UTC (rev 123756)
@@ -166,6 +166,9 @@
void clearAndUpdateTileOfNotRenderedRegion(const TileIndex&, BackingStoreTile*, const Platform::IntRectRegion&, const Platform::IntRect& backingStoreRect, bool update = true);
bool isCurrentVisibleJob(const TileIndex&, BackingStoreTile*, const Platform::IntRect& backingStoreRect) const;
+ // Not thread safe. Call only when threads are in sync.
+ void clearRenderedRegion(BackingStoreTile*, const Platform::IntRectRegion&);
+
// Responsible for scrolling the backing store and updating the
// tile matrix geometry.
void scrollBackingStore(int deltaX, int deltaY);
Modified: trunk/Source/WebKit/blackberry/ChangeLog (123755 => 123756)
--- trunk/Source/WebKit/blackberry/ChangeLog 2012-07-26 16:17:21 UTC (rev 123755)
+++ trunk/Source/WebKit/blackberry/ChangeLog 2012-07-26 16:22:25 UTC (rev 123756)
@@ -1,5 +1,41 @@
2012-07-26 Arvid Nilsson <anils...@rim.com>
+ [BlackBerry] Refactor BackingStorePrivate::BackingStorePrivate::clearAndUpdateTileOfNotRenderedRegion() to avoid touching tile frontbuffer
+ https://bugs.webkit.org/show_bug.cgi?id=92095
+
+ Reviewed by George Staikos.
+
+ PR: 141439
+ Specifically, we want to avoid changing the rendered region of the
+ front buffer without proper synchronization.
+
+ The method is trying to force checkerboard to appear on screen because
+ an area was invalidated but the render job was dropped, so the tile
+ contents are now invalid.
+
+ Unfortunately it did this in a way which is not thread safe. Fixed by
+ making it thread safe, in a way that minimizes memory bandwidth usage.
+
+ Instead of using the customary sequence of copy-back, modify and swap,
+ we send a synchronous message to the compositing thread to avoid the
+ copy-back step and save memory bandwidth. The trade-off is that the
+ WebKit thread might wait a little longer for the compositing thread
+ than it would from a waitForCurrentMessage() call.
+
+ The way we synchronize this is rather expensive for the WebKit thread,
+ and this method is called rather carelessly, so add various early
+ returns to avoid doing it redundantly.
+
+ Internally reviewed by Jakob Petsovits and Adam Treat.
+
+ * Api/BackingStore.cpp:
+ (BlackBerry::WebKit::BackingStorePrivate::indexOfTile):
+ (BlackBerry::WebKit::BackingStorePrivate::clearAndUpdateTileOfNotRenderedRegion):
+ * Api/BackingStore_p.h:
+ (BackingStorePrivate):
+
+2012-07-26 Arvid Nilsson <anils...@rim.com>
+
[BlackBerry] Refactor BackingStorePrivate::render() to avoid touching tile frontbuffer
https://bugs.webkit.org/show_bug.cgi?id=91989
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes
