Bug 178248 - SVGPathElement should cache the built-up Path of its non animating pathByteStream()
Summary: SVGPathElement should cache the built-up Path of its non animating pathByteSt...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-12 18:21 PDT by Said Abou-Hallawa
Modified: 2017-11-12 22:54 PST (History)
8 users (show)

See Also:


Attachments
Patch (24.49 KB, patch)
2017-10-13 15:18 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.49 MB, application/zip)
2017-10-13 17:06 PDT, Build Bot
no flags Details
Patch (26.84 KB, patch)
2017-10-13 17:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.51 MB, application/zip)
2017-10-13 18:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (2.54 MB, application/zip)
2017-10-13 18:49 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (2.39 MB, application/zip)
2017-10-13 19:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (4.20 MB, application/zip)
2017-10-13 19:59 PDT, Build Bot
no flags Details
Patch (28.18 KB, patch)
2017-10-14 21:06 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (992.42 KB, application/zip)
2017-10-14 22:22 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.16 MB, application/zip)
2017-10-14 22:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.79 MB, application/zip)
2017-10-14 22:33 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (977.64 KB, application/zip)
2017-10-14 22:44 PDT, Build Bot
no flags Details
Patch (29.86 KB, patch)
2017-10-15 23:40 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (28.51 KB, patch)
2017-10-20 18:26 PDT, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2017-10-12 18:21:02 PDT
Instead of creating a Path object from the non animating pathByteStream() every time we need to updatePathFromPathElement(), the Path object can be cached once it is created and used for later calls.
Comment 1 Radar WebKit Bug Importer 2017-10-12 18:21:45 PDT
<rdar://problem/34968657>
Comment 2 Said Abou-Hallawa 2017-10-13 15:18:56 PDT
Created attachment 323753 [details]
Patch
Comment 3 Build Bot 2017-10-13 17:06:09 PDT
Comment on attachment 323753 [details]
Patch

Attachment 323753 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4851900

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html
Comment 4 Build Bot 2017-10-13 17:06:10 PDT
Created attachment 323775 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 5 Said Abou-Hallawa 2017-10-13 17:06:15 PDT
Created attachment 323776 [details]
Patch
Comment 6 Build Bot 2017-10-13 18:33:19 PDT
Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4852744

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html
Comment 7 Build Bot 2017-10-13 18:33:21 PDT
Created attachment 323784 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2017-10-13 18:49:27 PDT
Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4852836

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html
Comment 9 Build Bot 2017-10-13 18:49:28 PDT
Created attachment 323787 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 10 Build Bot 2017-10-13 19:24:43 PDT
Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4852943

New failing tests:
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
css3/masking/clip-path-reference-restore.html
svg/dom/SVGPathSegList-segment-modification.svg
svg/carto.net/tabgroup.svg
svg/dom/SVGPathSegList-xml-dom-synchronization2.xhtml
svg/repaint/svgsvgelement-repaint-children.html
Comment 11 Build Bot 2017-10-13 19:24:44 PDT
Created attachment 323790 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 12 Build Bot 2017-10-13 19:59:03 PDT
Comment on attachment 323776 [details]
Patch

Attachment 323776 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4853127

New failing tests:
imported/blink/svg/custom/viewport-resource-inval.svg
css3/masking/clip-path-reference-restore.html
svg/W3C-SVG-1.1-SE/paths-dom-02-f.svg
Comment 13 Build Bot 2017-10-13 19:59:04 PDT
Created attachment 323792 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 14 Said Abou-Hallawa 2017-10-14 21:06:26 PDT
Created attachment 323835 [details]
Patch
Comment 15 Build Bot 2017-10-14 22:22:01 PDT
Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/4860763

New failing tests:
css3/masking/clip-path-reference-restore.html
Comment 16 Build Bot 2017-10-14 22:22:02 PDT
Created attachment 323836 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-10-14 22:23:50 PDT
Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4860770

New failing tests:
css3/masking/clip-path-reference-restore.html
Comment 18 Build Bot 2017-10-14 22:23:51 PDT
Created attachment 323837 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-10-14 22:33:23 PDT
Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4860788

New failing tests:
css3/masking/clip-path-reference-restore.html
Comment 20 Build Bot 2017-10-14 22:33:25 PDT
Created attachment 323838 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Build Bot 2017-10-14 22:44:09 PDT
Comment on attachment 323835 [details]
Patch

Attachment 323835 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4860794

New failing tests:
imported/blink/svg/custom/viewport-resource-inval.svg
css3/masking/clip-path-reference-restore.html
Comment 22 Build Bot 2017-10-14 22:44:11 PDT
Created attachment 323839 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 23 Said Abou-Hallawa 2017-10-15 23:40:39 PDT
Created attachment 323875 [details]
Patch
Comment 24 Simon Fraser (smfr) 2017-10-16 11:36:11 PDT
Comment on attachment 323875 [details]
Patch

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

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:68
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/cairo/PathCairo.cpp:106
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:119
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/cg/PathCG.cpp:139
> +    m_path = other.m_path;
> +    other.m_path = nullptr;

Can this just be m_path = WTFMove(other.m_path)?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:173
> +    m_path = other.m_path;
> +    m_activePath = other.m_activePath;
> +    m_activePathGeometry = other.m_activePathGeometry;
> +    other.m_path = nullptr;
> +    other.m_activePath = nullptr;
> +    other.m_activePathGeometry = nullptr;

Can these WTFMove?

> Source/WebCore/platform/graphics/win/PathDirect2D.cpp:190
> +    m_path = other.m_path;
> +    m_activePath = other.m_activePath;
> +    m_activePathGeometry = other.m_activePathGeometry;

Can these WTFMove?

> Source/WebCore/rendering/svg/SVGPathData.cpp:49
> +        return path;

Maybe return { } and move the declaration of path lower down.

> Source/WebCore/rendering/svg/SVGPathData.cpp:66
> +        return path;

Maybe return { } and move the declaration of path lower down.

> Source/WebCore/rendering/svg/SVGPathData.cpp:119
> +        return path;

Maybe return { } and move the declaration of path lower down.

> Source/WebCore/rendering/svg/SVGPathData.cpp:134
> +        return path;

Maybe return { } and move the declaration of path lower down.
Comment 25 Said Abou-Hallawa 2017-10-20 18:26:56 PDT
Created attachment 324478 [details]
Patch
Comment 26 Said Abou-Hallawa 2017-10-20 18:36:46 PDT
Comment on attachment 323875 [details]
Patch

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

>> Source/WebCore/platform/graphics/cg/PathCG.cpp:119
>> +    other.m_path = nullptr;
> 
> Can this just be m_path = WTFMove(other.m_path)?

A raw pointer does not necessarily represent an ownership relationship. So WTFMove() or std::move does not set the raw pointer to nullptr after moving it. m_path = WTFMove(other.m_path) can only work if PlatformPathPtr is a std::unique_ptr<> or a RefPtr<>.

>> Source/WebCore/rendering/svg/SVGPathData.cpp:49
>> +        return path;
> 
> Maybe return { } and move the declaration of path lower down.

Done.
Comment 27 WebKit Commit Bot 2017-10-20 19:33:57 PDT
Comment on attachment 324478 [details]
Patch

Clearing flags on attachment: 324478

Committed r223804: <https://trac.webkit.org/changeset/223804>
Comment 28 WebKit Commit Bot 2017-10-20 19:33:59 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Darin Adler 2017-11-12 22:54:27 PST
Comment on attachment 323875 [details]
Patch

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

>>> Source/WebCore/platform/graphics/cg/PathCG.cpp:119
>>> +    other.m_path = nullptr;
>> 
>> Can this just be m_path = WTFMove(other.m_path)?
> 
> A raw pointer does not necessarily represent an ownership relationship. So WTFMove() or std::move does not set the raw pointer to nullptr after moving it. m_path = WTFMove(other.m_path) can only work if PlatformPathPtr is a std::unique_ptr<> or a RefPtr<>.

For raw pointers, a good idiom is:

    m_path = std::exchange(other.m_path, nullptr);