- Revision
- 274688
- Author
- [email protected]
- Date
- 2021-03-18 16:42:55 -0700 (Thu, 18 Mar 2021)
Log Message
Avoid copying item data when iterating in-process display lists
https://bugs.webkit.org/show_bug.cgi?id=223471
Reviewed by Simon Fraser.
Source/WebCore:
As a performance optimization, in the case where display lists are built up without item buffer clients that are
responsible for encoding and decoding display list data, all display list items are constructed (and exist)
directly within the item buffer. In this scenario, there is no need to additionally decode or validate inline
items by copying them into a separate buffer, so we can simply skip this step and just use the memory address of
the item inside `m_allocatedBuffers` as the curren item handle when iterating through in-process display lists.
This skips almost all of the work done underneath `DisplayList::iterator::updateCurrentItem()` in the web
process on the Design subtest of MotionMark. Covered by existing API tests (one of which needs a slight
adjustment).
* platform/graphics/displaylists/DisplayList.cpp:
(WebCore::DisplayList::DisplayList::iterator::atEnd const):
(WebCore::DisplayList::DisplayList::iterator::updateCurrentItem):
(WebCore::DisplayList::DisplayList::iterator::advance):
Rearrange this logic so that we only decode or copy out to a side buffer and perform validation in the case
where we have an item buffer client. Otherwise, we know that the items already exist inside the item buffer's
`m_allocatedBuffers`, so we can simply use the current memory address (`m_cursor`) as the item handle.
(WebCore::DisplayList::DisplayList::iterator::clearCurrentItem):
Only destroy the item at `m_currentBufferForItem` if it doesn't already exist in the display list's item buffer.
(that is, if the item buffer requires a reading client). The items within the item buffer are destroyed upon
clearing the `DisplayList`, so running this logic would result in a double-free.
(WebCore::DisplayList::DisplayList::iterator::moveToEnd):
(WebCore::DisplayList::DisplayList::iterator::moveCursorToStartOfCurrentBuffer):
* platform/graphics/displaylists/DisplayList.h:
(WebCore::DisplayList::DisplayList::iterator::itemBuffer const):
Drive-by refactoring: add a helper method to grab the display list's `ItemBuffer`, and use it in several places
above.
Tools:
Adjust an existing API test that exercises inline display list item validation by assigning a reading client to
the display list.
* TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp:
(TestWebKitAPI::TEST):
Move the `ReadingClient` and `WritingClient` out of the scope of `OutOfLineItemDecodingFailure`, so that the
other API test (`InlineItemValidationFailure`) can also use it.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (274687 => 274688)
--- trunk/Source/WebCore/ChangeLog 2021-03-18 23:40:54 UTC (rev 274687)
+++ trunk/Source/WebCore/ChangeLog 2021-03-18 23:42:55 UTC (rev 274688)
@@ -1,3 +1,43 @@
+2021-03-18 Wenson Hsieh <[email protected]>
+
+ Avoid copying item data when iterating in-process display lists
+ https://bugs.webkit.org/show_bug.cgi?id=223471
+
+ Reviewed by Simon Fraser.
+
+ As a performance optimization, in the case where display lists are built up without item buffer clients that are
+ responsible for encoding and decoding display list data, all display list items are constructed (and exist)
+ directly within the item buffer. In this scenario, there is no need to additionally decode or validate inline
+ items by copying them into a separate buffer, so we can simply skip this step and just use the memory address of
+ the item inside `m_allocatedBuffers` as the curren item handle when iterating through in-process display lists.
+
+ This skips almost all of the work done underneath `DisplayList::iterator::updateCurrentItem()` in the web
+ process on the Design subtest of MotionMark. Covered by existing API tests (one of which needs a slight
+ adjustment).
+
+ * platform/graphics/displaylists/DisplayList.cpp:
+ (WebCore::DisplayList::DisplayList::iterator::atEnd const):
+ (WebCore::DisplayList::DisplayList::iterator::updateCurrentItem):
+ (WebCore::DisplayList::DisplayList::iterator::advance):
+
+ Rearrange this logic so that we only decode or copy out to a side buffer and perform validation in the case
+ where we have an item buffer client. Otherwise, we know that the items already exist inside the item buffer's
+ `m_allocatedBuffers`, so we can simply use the current memory address (`m_cursor`) as the item handle.
+
+ (WebCore::DisplayList::DisplayList::iterator::clearCurrentItem):
+
+ Only destroy the item at `m_currentBufferForItem` if it doesn't already exist in the display list's item buffer.
+ (that is, if the item buffer requires a reading client). The items within the item buffer are destroyed upon
+ clearing the `DisplayList`, so running this logic would result in a double-free.
+
+ (WebCore::DisplayList::DisplayList::iterator::moveToEnd):
+ (WebCore::DisplayList::DisplayList::iterator::moveCursorToStartOfCurrentBuffer):
+ * platform/graphics/displaylists/DisplayList.h:
+ (WebCore::DisplayList::DisplayList::iterator::itemBuffer const):
+
+ Drive-by refactoring: add a helper method to grab the display list's `ItemBuffer`, and use it in several places
+ above.
+
2021-03-18 Commit Queue <[email protected]>
Unreviewed, reverting r274669.
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp (274687 => 274688)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp 2021-03-18 23:40:54 UTC (rev 274687)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.cpp 2021-03-18 23:42:55 UTC (rev 274688)
@@ -325,7 +325,7 @@
if (m_displayList.isEmpty() || !m_isValid)
return true;
- auto& items = *m_displayList.m_items;
+ auto& items = *itemBuffer();
auto endCursor = items.m_writableBuffer.data + items.m_writtenNumberOfBytes;
return m_cursor == endCursor;
}
@@ -337,7 +337,7 @@
if (atEnd())
return;
- auto& items = *m_displayList.m_items;
+ auto& items = *itemBuffer();
auto itemType = static_cast<ItemType>(m_cursor[0]);
if (isDrawingItem(itemType) && !m_displayList.m_drawingItemExtents.isEmpty()) {
m_currentExtent = m_displayList.m_drawingItemExtents[m_drawingItemIndex];
@@ -347,20 +347,25 @@
auto* client = items.m_readingClient;
auto paddedSizeOfTypeAndItem = paddedSizeOfTypeAndItemInBytes(itemType);
- m_currentBufferForItem = paddedSizeOfTypeAndItem <= sizeOfFixedBufferForCurrentItem ? m_fixedBufferForCurrentItem : reinterpret_cast<uint8_t*>(fastMalloc(paddedSizeOfTypeAndItem));
- if (!isInlineItem(itemType) && client) {
- auto dataLength = reinterpret_cast<uint64_t*>(m_cursor)[1];
- auto* startOfData = m_cursor + 2 * sizeof(uint64_t);
- auto decodedItemHandle = client->decodeItem(startOfData, dataLength, itemType, m_currentBufferForItem);
- if (UNLIKELY(!decodedItemHandle))
- m_isValid = false;
+ if (client) {
+ m_currentBufferForItem = paddedSizeOfTypeAndItem <= sizeOfFixedBufferForCurrentItem ? m_fixedBufferForCurrentItem : reinterpret_cast<uint8_t*>(fastMalloc(paddedSizeOfTypeAndItem));
+ if (isInlineItem(itemType)) {
+ if (UNLIKELY(!ItemHandle { m_cursor }.safeCopy({ m_currentBufferForItem })))
+ m_isValid = false;
- m_currentBufferForItem[0] = static_cast<uint8_t>(itemType);
- m_currentItemSizeInBuffer = 2 * sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), dataLength);
+ m_currentItemSizeInBuffer = paddedSizeOfTypeAndItem;
+ } else {
+ auto dataLength = reinterpret_cast<uint64_t*>(m_cursor)[1];
+ auto* startOfData = m_cursor + 2 * sizeof(uint64_t);
+ auto decodedItemHandle = client->decodeItem(startOfData, dataLength, itemType, m_currentBufferForItem);
+ if (UNLIKELY(!decodedItemHandle))
+ m_isValid = false;
+
+ m_currentBufferForItem[0] = static_cast<uint8_t>(itemType);
+ m_currentItemSizeInBuffer = 2 * sizeof(uint64_t) + roundUpToMultipleOf(alignof(uint64_t), dataLength);
+ }
} else {
- if (UNLIKELY(!ItemHandle { m_cursor }.safeCopy({ m_currentBufferForItem })))
- m_isValid = false;
-
+ m_currentBufferForItem = m_cursor;
m_currentItemSizeInBuffer = paddedSizeOfTypeAndItem;
}
}
@@ -372,7 +377,7 @@
m_cursor += m_currentItemSizeInBuffer;
- if (m_cursor == m_currentEndOfBuffer && m_readOnlyBufferIndex < m_displayList.m_items->m_readOnlyBuffers.size()) {
+ if (m_cursor == m_currentEndOfBuffer && m_readOnlyBufferIndex < itemBuffer()->m_readOnlyBuffers.size()) {
m_readOnlyBufferIndex++;
moveCursorToStartOfCurrentBuffer();
}
@@ -382,7 +387,8 @@
void DisplayList::iterator::clearCurrentItem()
{
- if (m_currentBufferForItem) {
+ auto items = itemBuffer();
+ if (items && items->m_readingClient && m_currentBufferForItem) {
if (LIKELY(m_isValid))
ItemHandle { m_currentBufferForItem }.destroy();
@@ -396,7 +402,7 @@
void DisplayList::iterator::moveToEnd()
{
- if (auto& items = m_displayList.m_items) {
+ if (auto items = itemBuffer()) {
m_cursor = items->m_writableBuffer.data + items->m_writtenNumberOfBytes;
m_currentEndOfBuffer = m_cursor;
m_readOnlyBufferIndex = items->m_readOnlyBuffers.size();
@@ -405,7 +411,7 @@
void DisplayList::iterator::moveCursorToStartOfCurrentBuffer()
{
- auto& items = m_displayList.m_items;
+ auto items = itemBuffer();
if (!items)
return;
Modified: trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h (274687 => 274688)
--- trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h 2021-03-18 23:40:54 UTC (rev 274687)
+++ trunk/Source/WebCore/platform/graphics/displaylists/DisplayList.h 2021-03-18 23:42:55 UTC (rev 274688)
@@ -142,6 +142,8 @@
WEBCORE_EXPORT void advance();
bool atEnd() const;
+ ItemBuffer* itemBuffer() const { return m_displayList.itemBufferIfExists(); }
+
const DisplayList& m_displayList;
uint8_t* m_cursor { nullptr };
size_t m_readOnlyBufferIndex { 0 };
Modified: trunk/Tools/ChangeLog (274687 => 274688)
--- trunk/Tools/ChangeLog 2021-03-18 23:40:54 UTC (rev 274687)
+++ trunk/Tools/ChangeLog 2021-03-18 23:42:55 UTC (rev 274688)
@@ -1,3 +1,19 @@
+2021-03-18 Wenson Hsieh <[email protected]>
+
+ Avoid copying item data when iterating in-process display lists
+ https://bugs.webkit.org/show_bug.cgi?id=223471
+
+ Reviewed by Simon Fraser.
+
+ Adjust an existing API test that exercises inline display list item validation by assigning a reading client to
+ the display list.
+
+ * TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp:
+ (TestWebKitAPI::TEST):
+
+ Move the `ReadingClient` and `WritingClient` out of the scope of `OutOfLineItemDecodingFailure`, so that the
+ other API test (`InlineItemValidationFailure`) can also use it.
+
2021-03-18 Cameron McCormack <[email protected]>
Add another email for myself in contributors.json
Modified: trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp (274687 => 274688)
--- trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp 2021-03-18 23:40:54 UTC (rev 274687)
+++ trunk/Tools/TestWebKitAPI/Tests/WebCore/cg/DisplayListTestsCG.cpp 2021-03-18 23:42:55 UTC (rev 274688)
@@ -81,31 +81,31 @@
}
}
-TEST(DisplayListTests, OutOfLineItemDecodingFailure)
-{
- static ItemBufferIdentifier globalBufferIdentifier = ItemBufferIdentifier::generate();
+static ItemBufferIdentifier globalBufferIdentifier = ItemBufferIdentifier::generate();
- class ReadingClient : public ItemBufferReadingClient {
- private:
- Optional<ItemHandle> WARN_UNUSED_RETURN decodeItem(const uint8_t*, size_t, ItemType type, uint8_t*) final
- {
- EXPECT_EQ(type, ItemType::FillPath);
- return WTF::nullopt;
- }
- };
+class ReadingClient : public ItemBufferReadingClient {
+private:
+ Optional<ItemHandle> WARN_UNUSED_RETURN decodeItem(const uint8_t*, size_t, ItemType type, uint8_t*) final
+ {
+ EXPECT_EQ(type, ItemType::FillPath);
+ return WTF::nullopt;
+ }
+};
- class WritingClient : public ItemBufferWritingClient {
- private:
- ItemBufferHandle createItemBuffer(size_t capacity) final
- {
- EXPECT_LT(capacity, globalItemBufferCapacity);
- return { globalBufferIdentifier, globalItemBuffer, globalItemBufferCapacity };
- }
+class WritingClient : public ItemBufferWritingClient {
+private:
+ ItemBufferHandle createItemBuffer(size_t capacity) final
+ {
+ EXPECT_LT(capacity, globalItemBufferCapacity);
+ return { globalBufferIdentifier, globalItemBuffer, globalItemBufferCapacity };
+ }
- RefPtr<SharedBuffer> encodeItem(ItemHandle) const final { return SharedBuffer::create(); }
- void didAppendData(const ItemBufferHandle&, size_t, DidChangeItemBuffer) final { }
- };
+ RefPtr<SharedBuffer> encodeItem(ItemHandle) const final { return SharedBuffer::create(); }
+ void didAppendData(const ItemBufferHandle&, size_t, DidChangeItemBuffer) final { }
+};
+TEST(DisplayListTests, OutOfLineItemDecodingFailure)
+{
FloatRect contextBounds { 0, 0, contextWidth, contextHeight };
auto colorSpace = adoptCF(CGColorSpaceCreateWithName(kCGColorSpaceSRGB));
@@ -144,6 +144,8 @@
GraphicsContext context { cgContext.get() };
DisplayList list;
+ ReadingClient reader;
+ list.setItemBufferClient(&reader);
list.append<FlushContext>(FlushIdentifier { });
Replayer replayer { context, list };
