Some DOM mutations of SVG documents ignore suspendRedraw

RESOLVED FIXED in mozilla13

Status

()

Core
SVG
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: heycam, Assigned: Robert Longson)

Tracking

Trunk
mozilla13
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
For example, changing a presentation attribute like fill="" or removing an element from the document will cause a repaint, while changing transform="" or adding a new element does honour suspendRedraw.

It's not clear whether we really want to suspend redraw in these cases: the (un)suspendRedraw API is probably misguided, and it could well be that tracking changes like the above is just not worth it.
Yeah, our redraw suspension is very simplistic: it defers certain SVG-specific notification, but doesn't do anything about generic style or DOM changes, which is what you're seeing above.

I'd be fine with implementing SuspendRedraw just like webkit does, fwiw (as in, make them no-ops with some "implement this sometime" comments).
(Reporter)

Comment 2

7 years ago
I made some tests for suspendRedraw in bug 616893 to help me be a bit more confident that I don't regress the current behaviour, which includes a couple of disabled tests that could be enabled if this bug is ever fixed.
I'd be totally OK with stubbing them out completely. It just doesn't seem like a good feature for the Web to me.
(Assignee)

Comment 4

7 years ago
It's slow too unless you're careful as it has to look up the outer SVG frame/element to determine whether we're suspended and that can take longer than the time you save not drawing unless you're doing lots of updates. See bug 422058 for some discussion.
(Reporter)

Updated

6 years ago
Blocks: 616893
(Assignee)

Comment 5

5 years ago
Created attachment 586121 [details] [diff] [review]
patch

This simplifies the suspend/unsuspend code and fixes it for all cases except the removal of frames. It includes the tests Cameron wrote in bug 616893. I'll need to raise a follow up for the frame removal case.
Attachment #586121 - Flags: review?(dholbert)
I'm not particularly familiar with the code touched by this patch (Suspend/Unsuspend redraw stuff).  If any other SVG people do know it (maybe jwatt or roc or bz?), perhaps they'd make a more appropriate reviewer?

(If no one fits that bill (or has cycles to take it), I'm still opening to reviewing this, though.)
Why aren't we just removing support for suspendRedraw?
(Assignee)

Updated

5 years ago
Attachment #586121 - Flags: review?(dholbert) → review?(roc)
(Assignee)

Comment 8

5 years ago
We may remove it from being set by DOM at some point but we still want to suppress redraws before we've got a viewport size. Most of this patch makes that more efficient so the number of invalidation requests once we decide to display is much reduced.

Other good stuff is that it removes member variables from text and glyphs and saves some memory there.

Also if you look at http://svg.kvalitne.cz/index2.htm and search for the 

Big Benchmarking of SVG Particles in Recent Browser Versions

article you'll see that suspendRedraw does help our performance and this patch speeds up the suspendRedraw benchmarks there by around 20%. It will likely help less once we complete all the SVG display list work and we could remove ability of DOM to suspend redraws then.
Attachment #586121 - Flags: review?(roc) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 9

5 years ago
Created attachment 588461 [details] [diff] [review]
update to tip
Assignee: nobody → longsonr
Attachment #586121 - Attachment is obsolete: true
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Why aren't we just removing support for suspendRedraw?

I'd say that these are powerful tools for a developer to take an active role on the redraw process, sometimes required for heavy operations.

As part of bug 614732 comment 38 (no separate bug yet, sorry) I've been idle researching performance of redraw operations under pressure and concluded that suspendRedraw helps obtaining about 40% improvement in my test case. (Really need to find some time to polish the testcases a bit and attach them somewhere.)
I would appreciate a separate bug for that. I think it's very likely that we can get the same performance improvements as you're getting from suspendRedraw by either a) improving Firefox or b) using a different technique in your code. We'd need to know more about your testcase to be sure.
(Assignee)

Comment 12

5 years ago
Created attachment 594412 [details] [diff] [review]
update to tip and fix to pass try

Pretty similar to the one you gave an r+ to previously but without some of the code to minimise redraws as that caused a reftest failure of one reftest on linux.
Attachment #588461 - Attachment is obsolete: true
Attachment #594412 - Flags: review?(roc)
Attachment #594412 - Flags: review?(roc) → review+
(Assignee)

Comment 13

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/6b00b520b304
(Assignee)

Updated

5 years ago
Flags: in-testsuite+
Target Milestone: --- → mozilla13
(Assignee)

Comment 14

5 years ago
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/924abf8b59d6 for what seems to be a windows only random reftest
https://hg.mozilla.org/mozilla-central/rev/924abf8b59d6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
and the original landing:
https://hg.mozilla.org/mozilla-central/rev/6b00b520b304
You need to log in before you can comment on or make changes to this bug.