Bug 208659 - Remove the optimization for discarding no operation DisplayList items between Save and Restore items
Summary: Remove the optimization for discarding no operation DisplayList items between...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Canvas (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Said Abou-Hallawa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-05 12:35 PST by Said Abou-Hallawa
Modified: 2021-05-05 11:36 PDT (History)
6 users (show)

See Also:

Attachments
Patch (7.88 KB, patch)
2020-03-05 12:36 PST, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch (7.90 KB, patch)
2020-03-05 13:22 PST, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Patch (10.91 KB, patch)
2020-03-05 15:35 PST, Said Abou-Hallawa 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Said Abou-Hallawa 2020-03-05 12:35:27 PST 
The restoreIndex was an opportunity of optimization. If all no drawing between the Save and Restore happens, this means these items are all void and can be deleted from the DisplayList. With the canvas drawing and with batching the DisplayList flushing, this causes a serious problem when we try to remove the items at and after currentState().saveItemIndex.
Comment 1 Said Abou-Hallawa 2020-03-05 12:36:59 PST 
Created attachment 392607 [details]
Patch
Comment 2 Simon Fraser (smfr) 2020-03-05 13:01:08 PST 
Comment on attachment 392607 [details]
Patch

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

Please file a bug to track adding this optimization back.

> Source/WebCore/ChangeLog:3
> +        DisplayList::Save item should not track the index of the corresponding Restore item

This title should be "remove the optimization for..."
Comment 3 Said Abou-Hallawa 2020-03-05 13:22:12 PST 
Created attachment 392614 [details]
Patch
Comment 4 WebKit Commit Bot 2020-03-05 14:22:16 PST 
The commit-queue encountered the following flaky tests while processing attachment 392614 [details]:

editing/spelling/spellcheck-paste-continuous-disabled.html bug 208016 (authors: g.czajkowski@samsung.com and mark.lam@apple.com)
The commit-queue is continuing to process your patch.
Comment 5 WebKit Commit Bot 2020-03-05 14:22:52 PST 
Comment on attachment 392614 [details]
Patch

Clearing flags on attachment: 392614

Committed r257945: <https://trac.webkit.org/changeset/257945>
Comment 6 WebKit Commit Bot 2020-03-05 14:22:53 PST 
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2020-03-05 14:23:15 PST 
<rdar://problem/60100226>
Comment 8 Jacob Uphoff 2020-03-05 14:33:01 PST 
Reverted r257945 for reason:

This causes tests to fail

Committed r257947: <https://trac.webkit.org/changeset/257947>
Comment 9 Simon Fraser (smfr) 2020-03-05 14:50:00 PST 
Namely:
displaylists/canvas-display-list.html
displaylists/extent-includes-shadow.html
displaylists/extent-includes-transforms.html
Comment 10 Said Abou-Hallawa 2020-03-05 15:08:59 PST 
(In reply to Simon Fraser (smfr) from comment #9)
> Namely:
> displaylists/canvas-display-list.html
> displaylists/extent-includes-shadow.html
> displaylists/extent-includes-transforms.html

Yes because DisplayList::Restore has no parameters now and operator<<(TextStream& ts, const Item&) should not call operator<<(TextStream& ts, const Save&) because this is going to call itself and we end up in infinite recursion.
Comment 11 Said Abou-Hallawa 2020-03-05 15:35:19 PST 
Created attachment 392635 [details]
Patch
Comment 12 WebKit Commit Bot 2020-03-05 16:30:44 PST 
Comment on attachment 392635 [details]
Patch

Clearing flags on attachment: 392635

Committed r257958: <https://trac.webkit.org/changeset/257958>
Comment 13 WebKit Commit Bot 2020-03-05 16:30:46 PST 
All reviewed patches have been landed.  Closing bug.