Bug 217568

Summary: [MotionMark] Add an inline path data type to represent a circular sector
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: WebCore Misc.Assignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: darin, sabouhallawa, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=217563
Attachments:
Description Flags
Work in progress
darin: review+
For EWS ews-feeder: commit-queue-

Description Wenson Hsieh 2020-10-10 22:42:58 PDT
Similar to the extant ArcData, except it represents the following path recipe:

- Move to (x, y)
- Arc with radius r centered at (x, y)
- Line segment back to (x, y)

This should allow us to encode the remaining ~20% or so of “complex” paths in the Canvas Arcs subtest as inline data, which should reduce memory overhead when computing bounding rects, encoding paths, etc. Since the vast majority of the remaining 80% of paths in this subtest are simple arcs (represented by ArcData), this should, in theory, allow us to avoid CGPath allocation in the web process during this subtest.

Still need to measure projected perf impact...
Comment 1 Wenson Hsieh 2020-10-11 11:50:42 PDT
> 
> Still need to measure projected perf impact...

Some early testing shows roughly 2-3%, on top of webkit.org/b/217563
Comment 2 Wenson Hsieh 2020-10-11 11:51:13 PDT
(In reply to Wenson Hsieh from comment #1)
> > 
> > Still need to measure projected perf impact...
> 
> Some early testing shows roughly 2-3%, on top of webkit.org/b/217563

...2-3% better, that is :)
Comment 3 Wenson Hsieh 2020-10-11 17:30:34 PDT
Created attachment 411081 [details]
Work in progress
Comment 4 Darin Adler 2020-10-11 19:10:13 PDT
Comment on attachment 411081 [details]
Work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=411081&action=review

> Source/WebCore/platform/graphics/InlinePathData.h:52
>      bool hasStart { false };
> +    bool endsAtStart { false };

Not great to have these be separate booleans. These two booleans together have only 3 valid states, not 4.

> Source/WebCore/platform/graphics/Path.cpp:270
> +            arc.center.x() + arc.radius * acosf(arc.endAngle),
> +            arc.center.y() + arc.radius * asinf(arc.endAngle)

I suggest using std::acos and std::asin instead of acosf and asinf
Comment 5 Wenson Hsieh 2020-10-11 19:26:56 PDT
Comment on attachment 411081 [details]
Work in progress

View in context: https://bugs.webkit.org/attachment.cgi?id=411081&action=review

>> Source/WebCore/platform/graphics/InlinePathData.h:52
>> +    bool endsAtStart { false };
> 
> Not great to have these be separate booleans. These two booleans together have only 3 valid states, not 4.

Good catch! `endsAtStart && !hasStart` should never be a possibility.

I'll fix this by adding an enum class like:

    enum class Type : uint8_t {
        ArcOnly,
        LineAndArc,
        ClosedLineAndArc
    };

…and then replacing `hasStart` and `endsAtStart` with:

    Type type { Type::ArcOnly };

>> Source/WebCore/platform/graphics/Path.cpp:270
>> +            arc.center.y() + arc.radius * asinf(arc.endAngle)
> 
> I suggest using std::acos and std::asin instead of acosf and asinf

Done!
Comment 6 Wenson Hsieh 2020-10-11 20:52:48 PDT
Created attachment 411093 [details]
For EWS
Comment 7 EWS 2020-10-12 08:01:13 PDT
commit-queue failed to commit attachment 411093 [details] to WebKit repository.
Comment 8 Wenson Hsieh 2020-10-12 09:24:43 PDT
Committed r268352: <https://trac.webkit.org/changeset/268352>
Comment 9 Radar WebKit Bug Importer 2020-10-12 09:25:23 PDT
<rdar://problem/70209839>