Closed Bug 616892 Opened 11 years ago Closed 10 years ago

Some DOM mutations of SVG documents ignore suspendRedraw


(Core :: SVG, defect)

Not set





(Reporter: heycam, Assigned: longsonr)




(1 file, 2 obsolete files)

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).
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.
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.
Blocks: 616893
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #586121 - Flags: review?(dholbert) → review?(roc)
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 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.
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch update to tip (obsolete) — Splinter Review
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.
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)
Flags: in-testsuite+
Target Milestone: --- → mozilla13
pushed for what seems to be a windows only random reftest
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.