Last Comment Bug 616892 - Some DOM mutations of SVG documents ignore suspendRedraw
: Some DOM mutations of SVG documents ignore suspendRedraw
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Robert Longson
:
:
Mentors:
Depends on:
Blocks: 616893
  Show dependency treegraph
 
Reported: 2010-12-05 16:15 PST by Cameron McCormack (:heycam)
Modified: 2012-02-05 04:09 PST (History)
5 users (show)
longsonr: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (54.67 KB, patch)
2012-01-05 09:51 PST, Robert Longson
roc: review+
Details | Diff | Splinter Review
update to tip (51.47 KB, patch)
2012-01-13 10:54 PST, Robert Longson
no flags Details | Diff | Splinter Review
update to tip and fix to pass try (48.12 KB, patch)
2012-02-04 02:37 PST, Robert Longson
roc: review+
Details | Diff | Splinter Review

Description Cameron McCormack (:heycam) 2010-12-05 16:15:13 PST
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.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2010-12-05 21:26:07 PST
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).
Comment 2 Cameron McCormack (:heycam) 2010-12-05 21:34:27 PST
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.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-12-06 00:10:35 PST
I'd be totally OK with stubbing them out completely. It just doesn't seem like a good feature for the Web to me.
Comment 4 Robert Longson 2010-12-06 04:38:36 PST
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.
Comment 5 Robert Longson 2012-01-05 09:51:58 PST
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.
Comment 6 Daniel Holbert [:dholbert] 2012-01-05 11:17:24 PST
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.)
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-05 12:53:48 PST
Why aren't we just removing support for suspendRedraw?
Comment 8 Robert Longson 2012-01-05 14:51:09 PST
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.
Comment 9 Robert Longson 2012-01-13 10:54:02 PST
Created attachment 588461 [details] [diff] [review]
update to tip
Comment 10 Helder "Lthere" Magalhães 2012-02-02 09:19:26 PST
(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.)
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-02 14:42:36 PST
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.
Comment 12 Robert Longson 2012-02-04 02:37:43 PST
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.
Comment 14 Robert Longson 2012-02-04 13:01:51 PST
pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/924abf8b59d6 for what seems to be a windows only random reftest
Comment 15 Ed Morley [:emorley] 2012-02-05 04:05:21 PST
https://hg.mozilla.org/mozilla-central/rev/924abf8b59d6
Comment 16 Ed Morley [:emorley] 2012-02-05 04:09:55 PST
and the original landing:
https://hg.mozilla.org/mozilla-central/rev/6b00b520b304

Note You need to log in before you can comment on or make changes to this bug.