Bug 217568 - [MotionMark] Add an inline path data type to represent a circular sector
Summary: [MotionMark] Add an inline path data type to represent a circular sector
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-10-10 22:42 PDT by Wenson Hsieh
Modified: 2020-10-12 09:25 PDT (History)
5 users (show)

See Also:


Attachments
Work in progress (8.83 KB, patch)
2020-10-11 17:30 PDT, Wenson Hsieh
darin: review+
Details | Formatted Diff | Diff
For EWS (10.64 KB, patch)
2020-10-11 20:52 PDT, Wenson Hsieh
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>