270257 - nsSVGElement::SetParent is expensive

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This seems to send various notifications to some sorts of listeners on the
nsSVGLength attributes (from nsSVGRectElement::ParentChainChanged()).

Can those be lazy, somehow?  Esp. in light of bug 270255...

All that's happening here is creating an SVG node, setting 5 attrs, and
inserting into the document; it looks like we do a fair amount of duplicated
work during that process.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

one thing that is expensive is that we end up with a callchain like this:

SetContext->DidModify->NotifyObservers->DidModifySVGObservable->NotifyObserves->
nsSVGElement::DidModifySVGObservale->nsSVGElement::SetAttrAndNotify

In other words, we end up setting as reall attributes all 'mapped attributes'
that we call SetContext on.

This is both expensive and wrong since those attributes shouldn't show up in the
DOM unless explicitly set.

Comment 2

[Jonathan Watt [:jwatt]]

Actually I don't think we should call Will/DidModify in nsSVGLength::SetContext
at all. To expand the call chain Jonas mentioned, the stack that results in
SetAttrAndNotify being called when inserting an nsSVGRect element into a
document is as follows.

Object               Method
------               ------
nsSVGRect            nsSVGElement::SetAttrAndNotify()
nsSVGRect            nsSVGElement::DidModifySVGObservable()
nsSVGAnimatedLength  nsSVGValue::NotifyObservers()
nsSVGAnimatedLength  nsSVGValue::DidModify()
nsSVGAnimatedLength  nsSVGAnimatedLength::DidModifySVGObservable()
nsSVGLength          nsSVGValue::NotifyObservers()
nsSVGLength          nsSVGValue::DidModify()
nsSVGLength          nsSVGLength::SetContext()
nsSVGRect            nsSVGRectElement::ParentChainChanged()
nsSVGRect            nsSVGElement::SetParent()

I don't think we should call Will/DidModify for changes further *up* a parent
chain - only for changes further *down* it. When one object watches another, it
should only expect to be notified if that object or one of its decendents
changes. I don't think nsSVGLength::SetContext should be an exception to this
rule. The nsSVGLength object itself doesn't actually change as a result of one
of its ancestors being inserted into a new coordinate "context".

Whenever SetContext is called, it's because the nsSVGLength or one of it's
ancestors is being inserted into an SVG document fragment, or because it is
being inserted/removed from an nsSVGLengthList object. The function that carries
out the insertion should make sure any necessary notifications are sent out.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: don't Will/DidModify in SetContext

Comment 4

[Jonathan Watt [:jwatt]]

Comment on attachment 168073 [details] [diff] [review]
don't Will/DidModify in SetContext

Alex, can you review please.

Comment 5

[Alex Fritze]

Comment on attachment 168073 [details] [diff] [review]
don't Will/DidModify in SetContext

As discussed on irc, this won't work.
For percentage lengths, the 'real' user-unit value is determined by looking at
the context, and most observers expect to be informed of a change in the 'real'
value of a length.
The quick and dirty workaround would be to add a
'we-are-changing-the-context-not-the-actual-specified-value'-flag to
WillModify/DidModify.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we really need to notify the frame about parentchanges? It seems like
whenever the parent is changed we will move the node out of the old position in
the tree and into the new. During the moving out isn't the frame get destroyed,
and then recreated once the element is in the new position?

Comment 7

[Alex Fritze]

(In reply to comment #6)
> Do we really need to notify the frame about parentchanges? It seems like
> whenever the parent is changed we will move the node out of the old position in
> the tree and into the new. During the moving out isn't the frame get destroyed,
> and then recreated once the element is in the new position?

Good point. Maybe the patch is ok after all. 

Comment 8

[tor]

Checking in nsSVGLength.cpp;
/cvsroot/mozilla/content/svg/content/src/nsSVGLength.cpp,v  <--  nsSVGLength.cpp
new revision: 1.14; previous revision: 1.13
done

Comment 9

[tor]

Attachment: svg that crashes with this patch, is fine without it

Comment 10

[tor]

Backed out per irc conversation.

Comment 11

[Jonathan Watt [:jwatt]]

I've opened bug 274886 to address the issue of the SetAttrAndNotify call
separately since it no longer looks as if it will be fixed by removing the
WillModify/DidModify calls.

One thing we should do is check if mSpecifiedUnitType ==
SVG_LENGTHTYPE_PERCENTAGE before calling WillModify and DidModify in SetContext.
That doesn't make the notifications lazy, but it will eliminate a lot of
unnecessary notifications.

Comment 12

[Jonathan Watt [:jwatt]]

Attachment: selectively Will/DidModify in SetContext

Tim, can you check this doesn't crash you and review please?

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch

of course all lengths that aren't already in user units need a context to
determine what their user unit value is - thanks alex!

Comment 14

[Jonathan Watt [:jwatt]]

Attachment: patch

wrong file, sorry for the spam :-(

Comment 15

[Jonathan Watt [:jwatt]]

Attachment: patch

:-/ Win98 seems to have a stupid "feature" where it opens text files under
their DOS name. Again, sorry for the spam, here's the patch with the changes.

Comment 16

[Jonathan Watt [:jwatt]]

Attachment: only notify when necessary

I'm not sure about making things lazy, but how about we only notify our
observers when necessary?

bz: I would be interested to know if the checkin to fix the problem whereby
SetAttrAndNotify was always being called has helped performance any.

Comment 17

[Alex Fritze]

Comment on attachment 171842 [details] [diff] [review]
only notify when necessary

r=afri

Comment 18

[Jonathan Watt [:jwatt]]

Comment on attachment 171842 [details] [diff] [review]
only notify when necessary

Thanks Alex! Before this patch get's checked in I'd like to make sure that Tim
doesn't crash with it applied. Tim?

Comment 19

[tor]

My testcases seem to work fine with the patch now.

Comment 20

[Alex Fritze]

Checked in.

Comment 21

[Jonathan Watt [:jwatt]]

Boris, can I close this bug? If you can think of anything specific we could do
to make nsSVGElement::SetParent more efficient then I'll have a go at it here.
If you have any general problems with the notification system I think we should
open a separate bug.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Feel free to close it, yes.  I'll profile the testcase in bug 261974 with this
patch and file new bugs as needed....

Comment 23

[Boris Zbarsky [:bzbarsky]]

OK.  With a build from MOZ_CO_DATE="Thu Jan 20 22:08:11 CST 2005" the time spent
in SetParent is less than 0.6% of the total time spent on the testcase in bug
261974.  So looks like this helped, at least for those attributes!

Comment 24

[Jonathan Watt [:jwatt]]

bz, what do you get if you change all dimensions into millimeters:

    el.setAttribute("x", Math.random()*100 + "mm");
    el.setAttribute("y", Math.random()*100 + "mm");
    el.setAttribute("width", "1mm");
    el.setAttribute("height", "1mm");

That will give you poorer results. What I'd be interested to know though, is how
the results from that compair to your original results.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Using "mm" makes the time in SetParent about double.  I don't recall how this
compares to what I used to see... I suppose I could back this patch out (just
the last one, right?) and retest without it.

Comment 26

[Jonathan Watt [:jwatt]]

No, testing using "mm" should give you virtually the same results as with the
patch backed out. The patch only made things faster for unitless and "px" lengths.

What I was interested to know was how much the patch for bug 274886 has helped.
It essentially stopped us from calling SetAttrAndNotify, and I was interested to
know how much time had been spent in that function. Unfortunately, the patch to
that bug was rather more wide reaching (it touched 51 files) so if you don't
remember what you were getting before then I guess we'll never know. :-)

Comment 27

[Boris Zbarsky [:bzbarsky]]

Well... Given that I filed this bug, the number before was at least in the 5%
ballpark, and I seem to recall it being more like 10-15%.  I wasn't going to say
it because this patch didn't seem to help that much, but yes, the patch for bug
274886 would have helped a good bit.

Comment 28

[Jonathan Watt [:jwatt]]

Great, that's what I wanted to know. BTW, when you said the time *in*, did you
mean the time *under* (by which I mean the time spent from when SetParent
starts, to when it ends).

Comment 29

[Boris Zbarsky [:bzbarsky]]

Yes, all times are under.  Time in the function itself is pretty negligible.  ;)