302243 - Reduce SVG observer usage by nsSVGGradientFrame*

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

Comment 1

[tor]

Attachment: removing direct observation of values by gradients

Possible approach along the lines of bug 301628.

Comment 2

[Scooter Morris]

Attachment: Updated to trunk + some cleanup

I think this is ready for review

Comment 3

[Jonathan Watt [:jwatt]]

First thing's first. The following methods do/can exit without setting |mLoopFlag = PR_FALSE|

* nsSVGGradientFrame::AttributeChanged
* nsSVGGradientFrame::GetGradientUnits
* nsSVGGradientFrame::GetSpreadMethod

And at first it looks like the same is true of

* nsSVGRadialGradientFrame::GetFx
* nsSVGRadialGradientFrame::GetFy

when they call GetCx() and GetCy() respectively. However it's those called methods that reset mLoopFlag. Ugh.

Comment 4

[Jonathan Watt [:jwatt]]

You don't need to include nsISVGContainerFrame.h, do you?

Use |IsEmpty()| instead of |Length() == 0| since you're touching that line.

Will/DidModify have a default argument of |mod_other| so no need to pass it explicitly.

The |else| in nsSVGGradientFrame::AttributeChanged is bogus since the first |if| returns. You should also return after the call to checkURITarget().

Could you s/Content/Element/ to be more precice in variable names by renaming |lGradContent| to |linGradElement| etc. (e.g. same for rGradContent and gradContent)?

Instead of |if (gradContent == nsnull)| use |if (!gradElement)|. Same for lots of other places that have |== nsnull|.

I'm really not liking checkURITarget(nsIAtom *attr) - it's making the code harder to follow, and any tiny code size saving is cancelled out by the tiny increase in runtime overhead of having the extra function call. I'd much prefer if you could just check HasAttr explicitly at all it's call sites.

Comment 5

[Scooter Morris]

Attachment: Address review comments

Comment 6

[Scooter Morris]

((In reply to comment #4)
> You don't need to include nsISVGContainerFrame.h, do you?
Guess not.
> 
> Use |IsEmpty()| instead of |Length() == 0| since you're touching that line.
OK.
> 
> Will/DidModify have a default argument of |mod_other| so no need to pass it
> explicitly.
Done.
> 
> The |else| in nsSVGGradientFrame::AttributeChanged is bogus since the first
> |if| returns. You should also return after the call to checkURITarget().
Bogus is probably a little strong, but fine, I've removed the else.  The checkURITarget() was not needed, so I removed it.
> 
> Could you s/Content/Element/ to be more precice in variable names by renaming
> |lGradContent| to |linGradElement| etc. (e.g. same for rGradContent and
> gradContent)?
Fine.
> 
> Instead of |if (gradContent == nsnull)| use |if (!gradElement)|. Same for lots
> of other places that have |== nsnull|.
OK.
> 
> I'm really not liking checkURITarget(nsIAtom *attr) - it's making the code
> harder to follow, and any tiny code size saving is cancelled out by the tiny
> increase in runtime overhead of having the extra function call. I'd much prefer
> if you could just check HasAttr explicitly at all it's call sites.
> 
Since that's not really part of this bug, for the sake of cvs blame, I left that as is.  Other than that, all changes are in.

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 213945 [details] [diff] [review]
Address review comments

I don't think you want to change the return type of functions from PRBool to PRPackedBool.

The member |mLoopFlag| should typically be a quarter the size of |mSourceContent|. Can you swap their declared order so as not to undo the changes made in bug 328571 please?

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 213945 [details] [diff] [review]
Address review comments

We should return after |mNextGrad = nsnull| in nsSVGGradientFrame::AttributeChanged.

Would you mind renaming |nextGradStr| to just |href| and adding the following comment (or something similar) to the line above where it's declared?:
 // check if we reference another gradient to "inherit" its stops or attributes

In GetX (etc.) can you s/len/animLength/ and s/val/length/ (etc.)?

You also have some places where you've got variables named |rGrad| whereas in other places in the file you'd have named them |rGradElement|. I don't really care too much, but since you're touching those lines you may want to change that.

Other than that, this all looks great to me.

Comment 9

[Scooter Morris]

Attachment: Final review comments

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 214197 [details] [diff] [review]
Final review comments

You've not added a return after |mNextGrad = nsnull| in
nsSVGGradientFrame::AttributeChanged.

Comment 11

[Scooter Morris]

Attachment: Add return after href change

Oops -- nice catch, Jonathan.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 214206 [details] [diff] [review]
Add return after href change

+  if (nsSVGUtils::GetReferencedFrame(&nextGrad, targetURI, mContent, GetPresContext()->PresShell()) == NS_OK) {

if (NS_SUCCEEDED(...)) {

otherwise, looks good.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One thing that jumps out at me reading this code --- there's still a lot of duplicated code in GetX1/Y1/etc hopping around nextGrad and so forth, and it's going to be slow because I suppose that the renderer will normally need to get all the gradient parameters, and it's going to repeat a significant amount of work as it calls each method, especially when there is a next-gradient.

This could be fixed in various ways (not in this bug/patch of course). Perhaps the best would be to define a struct that contains all the parameters as floats, and one accessor method that grabs all the parameters at once, and have the renderer use that. This could happen in conjunction with deCOM of nsISVGGradient ... perferably just remove it and use nsSVGGradientFrame directly.

Comment 14

[Scooter Morris]

Attachment: Final patch -- includes NS_SUCCEEDED change

Comment 15

[Scooter Morris]

Checked in on trunk
r=jwatt/sr=roc

Comment 16

[Jonathan Watt [:jwatt]]

Apparently this made SVG slower (probably because we no longer cache the objects we need on the frame). CC'ing vlad and pav since I didn't really follow the conversation in #developers but it would be useful to have some sort of perspective on just how much slower things got. Can either of you guys comment on that?

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It's plausible because we're now doing considerably more work to fetch each rendering parameter, and we fetch several parameters every time we draw a gradient.

It would be useful to get some before-and-after profiles here with different kinds of gradients. In particular I wonder what kind of gradients are slower and what operations are particularly slow. Should get the SVG guys going with jprof...

Comment 18

[tor]

Inkscape output (like the kde gearflowers used in the benchmarking) makes heavy use of chained gradients.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

gah, so we're going through checkURITarget all the time and create a URI several times for each gradient paint?

http://lxr.mozilla.org/seamonkey/source/layout/svg/base/src/nsSVGGradientFrame.cpp#656

Comment 20

[Scooter Morris]

(In reply to comment #19)
> gah, so we're going through checkURITarget all the time and create a URI
> several times for each gradient paint?
> 
> http://lxr.mozilla.org/seamonkey/source/layout/svg/base/src/nsSVGGradientFrame.cpp#656
> 

No, actually, this is one area that we cache the next gradient pointer and reuse it without repeatedly creating URI's.  See http://lxr.mozilla.org/seamonkey/source/layout/svg/base/src/nsSVGGradientFrame.cpp#641
Once we've gone through checkURI and obtained the next gradient, we don't go through all of the URI stuff unless we null out mNextGrad for some reason.

Comment 21

[Jonathan Watt [:jwatt]]

Actually vlad's comments don't seem to have anything to do with this bug. From my IRC logs I see he noted SVG got slower and mentioned two consecutive sample points, 789.056 and 792.083, which are from 2006-03-09 11:27:00 and 2006-03-09 12:30:00 respectively. The check-in date for this patch was 2006-03-06 16:04 (three days earlier). The only gradient change around the time vlad was talking about was the check-in for bug 329836, but that occurred at 2006-03-09 10:02 which is actually before the first of the sample points he mentioned.

Putting that aside for a moment, the sample point directly preceding the two vlad mentioned was 794.250, which is higher than both of them. While the drop he mentioned may have been the source of the change in Tgfx across those two sample points, it doesn't seem to point to a notable slowdown in the SVG code when taken in context.

Taking more context, the average of the 8 times before and the 8 times after the point in question is 793.014 and 794.056 respectively. Looking at it this way the change looks more like a 1ms increase rather than a 3ms increase. Roughly it's a 0.13% increase in the time taken to render gearflowers.

Comment 22

[Jonathan Watt [:jwatt]]

In fact the check-in for bug 329836 won't have had any effect on the rendering time for gearflowers since that file never sets xlink:href using script. "Wasn't us guv."

Comment 23

[Jonathan Watt [:jwatt]]

The patch for this bug was checked in at 2006-03-06 16:04 as I noted earlier. The two sample points from before and after the check-in were 794.278 and 778.639 at 2006-03-06 15:24:00 and 2006-03-06 16:53:00 respectively. This would seem to indicate that the rendering of gearflowers actually speeded *up*. This seems to be confirmed by taking the mean of the eight sample points before and after the check-in which are 795.306 and 781.896 respectively.

Comment 24

[Scooter Morris]

Attachment: jprof performance data from gearflower section of Trender

Here some performance data that was taken after this patch was included