Bug 195743

Summary:

Optimize Region for single rectangle case

Product:

WebKit

Reporter:

Antti Koivisto <koivisto>

Component:

Platform

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cmarcelo, commit-queue, ews-watchlist, kondapallykalyan, luiz, noam, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
patch	none
patch	simon.fraser: review+
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none
patch	none

Antti Koivisto

Reported 2019-03-14 05:33:04 PDT

Instrumentation shows vast majority of Region objects consist of a single rectangle. However it always allocates the large Shape data structure. This makes it unsuitable to use as a member in any popular objects.

Attachments
patch (26.77 KB, patch) 2019-03-14 06:00 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (26.79 KB, patch) 2019-03-14 08:14 PDT, Antti Koivisto	simon.fraser: review+	Details Formatted Diff Diff
patch (29.96 KB, patch) 2019-03-15 02:27 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (28.41 KB, patch) 2019-03-15 03:21 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (29.02 KB, patch) 2019-03-15 03:53 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (26.35 KB, patch) 2019-03-15 06:17 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (26.47 KB, patch) 2019-03-15 06:22 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (27.07 KB, patch) 2019-03-15 07:00 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (27.76 KB, patch) 2019-03-15 07:23 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
patch (28.12 KB, patch) 2019-03-15 07:45 PDT, Antti Koivisto	no flags	Details Formatted Diff Diff
Show Obsolete (9) View All Add attachment proposed patch, testcase, etc.

Antti Koivisto

Comment 1 2019-03-14 06:00:19 PDT

Created attachment 364656 [details] patch

Antti Koivisto

Comment 2 2019-03-14 08:14:27 PDT

Created attachment 364659 [details] patch

Simon Fraser (smfr)

Comment 3 2019-03-14 08:57:05 PDT

Comment on attachment 364659 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=364659&action=review > Source/WebCore/platform/graphics/Region.cpp:60 > +Region::~Region() = default > Source/WebCore/platform/graphics/Region.cpp:85 > + if (!m_bounds.isEmpty()) > + rects.append(m_bounds); > + return rects; Since this is the common case, maybe reserveInitialCapacity(1) and uncheckedAppend here, avoiding wasted vector capacity. > Source/WebCore/platform/graphics/Region.h:189 > + return a.m_bounds == b.m_bounds && arePointingToEqualData(a.m_shape, b.m_shape); very nice > Source/WebCore/platform/graphics/Region.h:226 > + Optional<size_t> segmentIndex; > + decoder >> segmentIndex; I don't really find this use of Optional<> and decoder >> much clearer than testing for decode success directly.

Chris Dumez

Comment 4 2019-03-14 08:59:26 PDT

Comment on attachment 364659 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=364659&action=review >> Source/WebCore/platform/graphics/Region.h:226 >> + decoder >> segmentIndex; > > I don't really find this use of Optional<> and decoder >> much clearer than testing for decode success directly. It's not really about readability though. The reason we changed the way we decode was to not require a default constructor for the classes being decoded.

Simon Fraser (smfr)

Comment 5 2019-03-14 10:52:52 PDT

(In reply to Chris Dumez from comment #4) > It's not really about readability though. The reason we changed the way we > decode was to not require a default constructor for the classes being > decoded. Makes sense.

Antti Koivisto

Comment 6 2019-03-15 02:27:22 PDT

Created attachment 364783 [details] patch

Antti Koivisto

Comment 7 2019-03-15 03:21:36 PDT

Created attachment 364785 [details] patch

Antti Koivisto

Comment 8 2019-03-15 03:53:44 PDT

Created attachment 364789 [details] patch

Antti Koivisto

Comment 9 2019-03-15 06:17:28 PDT

Created attachment 364790 [details] patch