Bug 195743

Summary: Optimize Region for single rectangle case
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: PlatformAssignee: 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: P2 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

Description Antti Koivisto 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.
Comment 1 Antti Koivisto 2019-03-14 06:00:19 PDT
Created attachment 364656 [details]
patch
Comment 2 Antti Koivisto 2019-03-14 08:14:27 PDT
Created attachment 364659 [details]
patch
Comment 3 Simon Fraser (smfr) 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.
Comment 4 Chris Dumez 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.
Comment 5 Simon Fraser (smfr) 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.
Comment 6 Antti Koivisto 2019-03-15 02:27:22 PDT
Created attachment 364783 [details]
patch
Comment 7 Antti Koivisto 2019-03-15 03:21:36 PDT
Created attachment 364785 [details]
patch
Comment 8 Antti Koivisto 2019-03-15 03:53:44 PDT
Created attachment 364789 [details]
patch
Comment 9 Antti Koivisto 2019-03-15 06:17:28 PDT
Created attachment 364790 [details]
patch
Comment 10 Antti Koivisto 2019-03-15 06:22:16 PDT
Created attachment 364791 [details]
patch
Comment 11 Antti Koivisto 2019-03-15 07:00:54 PDT
Created attachment 364793 [details]
patch
Comment 12 Antti Koivisto 2019-03-15 07:23:36 PDT
Created attachment 364794 [details]
patch
Comment 13 Antti Koivisto 2019-03-15 07:45:37 PDT
Created attachment 364795 [details]
patch
Comment 14 WebKit Commit Bot 2019-03-15 08:43:26 PDT
Comment on attachment 364795 [details]
patch

Clearing flags on attachment: 364795

Committed r242995: <https://trac.webkit.org/changeset/242995>
Comment 15 WebKit Commit Bot 2019-03-15 08:43:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-03-15 08:44:17 PDT
<rdar://problem/48925684>