Last Comment Bug 666068 - Avoid calling UsingEffectsForFrame() on SVG content
: Avoid calling UsingEffectsForFrame() on SVG content
Status: RESOLVED FIXED
: perf
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Robert Longson
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-21 16:06 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2011-06-23 02:34 PDT (History)
5 users (show)
longsonr: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
like so? (1003 bytes, patch)
2011-06-22 08:57 PDT, Robert Longson
jwatt: review+
jmuizelaar: feedback+
Details | Diff | Splinter Review
hg changeset patch (931 bytes, patch)
2011-06-22 13:20 PDT, Robert Longson
no flags Details | Diff | Splinter Review

Description User image Jeff Muizelaar [:jrmuizel] 2011-06-21 16:06:51 PDT
On the SVG rotating tiger we spend about 0.9% of the time in UsingEffectsForFrame() which always returns false for svg content. We could avoid calling this by hanging a flag on the frame that would mark whether we are an SVG frame or not.
Comment 1 User image :Ehsan Akhgari 2011-06-21 16:08:57 PDT
I'm actually pretty surprised that we don't have this flag already.  It may also be useful in other pieces of our generic layout code where we do SVG specific hacks.
Comment 2 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-21 20:06:21 PDT
Where are we calling it from?  Invalidation, or something else?
Comment 3 User image Jeff Muizelaar [:jrmuizel] 2011-06-21 20:20:04 PDT
(In reply to comment #2)
> Where are we calling it from?  Invalidation, or something else?

Invalidation.
Comment 4 User image Boris Zbarsky [:bz] (still a bit busy) 2011-06-21 20:42:21 PDT
So bug 614732 might get us that "for free".
Comment 5 User image Jonathan Watt [:jwatt] 2011-06-21 21:25:12 PDT
Sounds like it's just a case of adding an |frame->IsFrameOfType(nsIFrame::eSVG)| check. Either that or wait to see if bug 614732 or its followups fix this as bz says.
Comment 6 User image Robert Longson 2011-06-22 04:40:22 PDT
Should we just reorder the UsingEffectsForFrame() method in nsSVGIntegrationUtils so that it checks for a SVG frame first rather than last?
Comment 7 User image Robert Longson 2011-06-22 08:57:43 PDT
Created attachment 541060 [details] [diff] [review]
like so?
Comment 8 User image Jeff Muizelaar [:jrmuizel] 2011-06-22 09:20:08 PDT
Comment on attachment 541060 [details] [diff] [review]
like so?

IsFrameOfType is still going to be a virtual call, but this patch seems like pure win.
Comment 9 User image Robert Longson 2011-06-22 13:20:27 PDT
Created attachment 541157 [details] [diff] [review]
hg changeset patch
Comment 10 User image :Ehsan Akhgari 2011-06-22 15:48:22 PDT
Landed on inbound.
Comment 11 User image Mounir Lamouri (:mounir) 2011-06-23 02:34:58 PDT
Pushed:
http://hg.mozilla.org/mozilla-central/rev/7bb02cc447e0

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