Currently we have lots of DidChangeXxx methods for sending out notifications when the SVG DOM is used to change SVGPathSegList objects etc.: http://hg.mozilla.org/mozilla-central/annotate/8b600831f115/content/svg/content/src/nsSVGElement.h#l176 The problem is that these methods call SetParsedAttr after the change has occurred, but under SetParsedAttr we call AttributeWillChange. For restyles to work correctly GetAttr needs to return the old value under AttributeWillChange, so our nsAttrValue can't point to and return the serialization of the SVG object that just changed. The way we handle this currently is to store the serialization of the SVG object in the nsAttrValue object so that under SetParsedAttr, GetAttr will be able to return the serialization of the old value instead of the serialization of the current object's value. The problem with this approach is that we need to serialize the SVG object (in the case of lists this can be a lot of serialization) every time the SVG object is changed, which in the case of a script going through building up a list or changing all its items can be a lot of times. It also means that we're using memory to store serializations that would otherwise be unnecessary. If we can redesign our notification system to send separate WillModify/DidModify notifications we should hopefully be able to get rid of all this serialization and extra memory use.

It's not just restyles for which we need to be able to get the old value under SetParsedAttr, it's also for mutation events, if they're sent.

I thought we'd also need to modify nsGenericElement::BeforeSetAttr and AfterSetAttr to take an nsAttrValue instead of a string, but it looks like we don't once bug 602759 is fixed, because at that point no SVG elements actually need BeforeSetAttr/AfterSetAttr to be called. Instead we simply need an attribute-setting path that doesn't call BeforeSetAttr/AfterSetAttr.

Taking this as it seems to be the root cause of bug 646510 which is assigned to me.

Note: when fixing this take care that the mIsBaseSet/mIsAttrSet/mAttrIsSet members of SVG types continue to be set correctly.

I'm just starting to dig into this. One clarification to check I'm on the right track--I guess we want to be able to wrap our SVG types in an nsAttrValue? I think we used to be able to do that with nsISVGValue but that interface is on the way out (in fact, after bug 602759 lands, I think only nsSVGStringProxyValue will use that interface). So perhaps we need another lightweight equivalent of that interface?

Yes. We may not need a common interface though, we can add as many new types as we need to ValueType with corresponding fields to MiscContainer. I think the cleanest way to do this (without creating new attribute-setting code paths that bypass BeforeSetAttr/AfterSetAttr sometimes) is to change nsGenericElement::SetParsedAttr to not serialize the new value to a string. I think this would require, at least: -- Allowing MaybeCheckSameAttrVal to work on parsed attributes instead of strings -- Changing BeforeSetAttr/AfterSetAttr to take nsAttrValues instead of strings I seem to remember more discussion about this in some other bug.

Just posting a bunch of WIP patches because I'm going to be away for a while. Basically so far I've done enums and lengths. Next is length lists. If those three turn out ok then I hope it will be fairly straightforward to update the other types by following a similar approach.

There's one test failure with this patch, but it's an existing bug I've yet to dig into (about not sending an "addition" notification when we touch the DOM interface of an enum that previously wasn't set)

Brian, I'm removing all the overrides of DidChangeXXX so that the versions in nsSVGElement can become non-virtual. That will save around 128 bytes for each element we create in the SVG namespace. The changes are not in direct conflict with what you're doing but they will bitrot these patches each time one of mine lands as you're modifying the overrides that I'm gradually removing. Once I've finished there won't be any overrides to modify. I have patches either landed or in review for everything except nsSVGFilters so far.

On reflection and with jwatt's help, that's 128 bytes per class not per instance.

(In reply to Robert Longson from comment #15) > Brian, > > I'm removing all the overrides of DidChangeXXX so that the versions in > nsSVGElement can become non-virtual. That will save around 128 bytes for > each element we create in the SVG namespace. The changes are not in direct > conflict with what you're doing but they will bitrot these patches each time > one of mine lands as you're modifying the overrides that I'm gradually > removing. Once I've finished there won't be any overrides to modify. I have > patches either landed or in review for everything except nsSVGFilters so far. Hi Robert, Thanks for the heads up. I'm currently on break, back in action next Monday. I'm already anticipating spending a bit of time unbitrotting this stuff so if you manage to get it landed by next week then it's absolutely no problem. Otherwise, I'll just keep in mind those changes and get ready to jump on board when they land. Thanks again.

I'm unbitrotting this but I've got a new test failure (in the newly added code and tests) due to bug 550047. Basically we used to run nsGenericElement::UnsetAttr BEFORE clearing the SVG data type (an nsSVGLength2 in the case I am debugging). So when we dispatched MutationEvent (AttrModified) for the attribute removal, the copied nsAttrValue we use to fill in the prevValue field of the event still pointed to the nsSVGLength2 with its value set. After nsGenericElement::UnsetAttr returned we'd go ahead and clear it. Since bug 550047 we now clear the internal SVG data types BEFORE calling nsGenericElement::UnsetAttr so when we go to dispatch the event the nsAttrValue now points to a cleared nsSVGLength2 and the prevValue field ends up being wrong. I'm thinking of doing: A) In nsSVGElement::UnsetAttrInternal, detect if we're going to dispatch an AttrModified event and, if so, serialise the attr value before clearing the SVG type. That way we should still get the right value in the prevValue for the (rare) case that the attribute has mutation listeners. I'm writing this up because I'm a little unsure about it. The serialisation seems a little hacky (although if we're dispatching the event, we're going to do some *some* serialisation, it's just now we end up using two string buffers). Also, it seems odd to call all these AttributeWillChange / BeforeSetAttr methods inside nsGenericElement::UnsetAttr when in fact the attribute has already changed (in the current case the SVG data type is already cleared and once we make nsAttrValues point to the SVG data types, the attribute too will have already been updated in effect. Even with the serialisation proposed in (A) the attribute will have at least changed format.) Somehow, it seems more consistent to have listeners to AttributeChanged and the like actually just look up the attribute in question, and if there's no nsAttrValue then act as if it is not set, rather than bypassing the attributes and going straight to the SVG internal types. But I wonder if AttributeChanged is triggering a lot of synchronous updates that all look up the SVG types directly. For now I'm going to give (A) a shot.

WIP patch 2. This time all folded up into one for easier cross-reference. * Unbitrotted * All tests now pass * SVGEnum, SVGLength, SVGLengthList now done Still about another dozen or so types to go but I think the above three are representative of the remaining types so hopefully it's just a matter of copying what they do and hopefully won't take too long.

do you need the aDoSetAttr argument in Will/DidChange. Can't you just not call the method at all if the argument is false?

(In reply to Robert Longson from comment #20) > do you need the aDoSetAttr argument in Will/DidChange. Can't you just not > call the method at all if the argument is false? I definitely want to get rid of that and do like you say. Were you going to make all those DidChangeXXX no longer virtual? If that's the case that would give me confidence to drop the unnecessary calls. Otherwise I need to go through and make sure we're not overriding DidChangeXXX anywhere before dropping the call.

bz, Jonas, it would be great if one of you (or both of you) can look at Brian's patch to make sure he's going in the right direction. Especially the nsGenericElement changes.

The nsGenericElement changes seem generally sane to me; the one concern is whether we end up making more string copies on setAttribute calls from JS, but I'm not sure we do in fact...

(In reply to Robert Longson from comment #20) > do you need the aDoSetAttr argument in Will/DidChange. Can't you just not > call the method at all if the argument is false? For what it's worth, I've started dropping these calls (and removing the aDoSetAttr) and making them non-virtual. Will post another patch on Friday--Thurs is public holiday here.

Updated WIP patch. Still to do: * nsSVGViewBox * SVGAnimatedNumberList * SVGAnimatedPointList * SVGAnimatedPathSegList * SVGAnimatedTransformList * See if I can make some sort of adaptor class for the SVG types so as not to pollute nsAttrValue with all these SVG types It's one big patch here but in 17 pieces in my queue. I'll post the pieces once it's about done.

Updated WIP patch. All types now done. Still to do: - Track down an apparent leak on windows 7 debug mochitest-chrome - Track down a test failure on one of the newly added tests on linux - Review all patches I did a rough perf comparison this afternoon between a recent try server build for this patch and the latest nightly with the test URL from bug 646510 and got the following results: Nightly build: ~13s until rendering complete + ~3s until browser becomes responsive TryServer build: <2s until rendering complete, immediately responsive I've made a few changes since but hopefully that's still more or less accurate.

Does it speed up Santa's Workshop at all?

It looks like this depends on bug 696078 and bug 698195 There are overrides to DidChangeValue in nsSVGFilters that need to go away before you can make them non-virtual. bug 696210 is also in the same area and can only land after bug 696078 and bug 698195), maybe this bug superseeds that though.

(In reply to Robert Longson from comment #28) > It looks like this depends on bug 696078 and bug 698195 There are overrides > to DidChangeValue in nsSVGFilters that need to go away before you can make > them non-virtual. bug 696210 is also in the same area and can only land > after bug 696078 and bug 698195), maybe this bug superseeds that though. I think bug 696078 is only dealing with changes to DidAnimateXXX? So no conflict? This patch doesn't touch DidChangeString--it leaves it as virtual so it shouldn't be dependent on bug 698195. Having a look at bug 696210, that bug's patch and this work overlap significantly. About half of the patch for bug 696210 is covered by this patch (the missing code being mostly the changes to DidChangeString and DidAnimateXXX). We can either: a) Land bug 696210 as is (after fixing bitrot from bug 698630) and rework this bug to fit on top, or b) Alter bug 696210 to just deal with the DidAnimateXXX changes, then deal with DidChangeString in bug 698195 and the rest of the DidChangeXXX methods here. That would save you having to fix most of the bitrot caused by bug 698630. I learn towards (b) but am ok with (a). What do you reckon?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > Does it speed up Santa's Workshop at all? Will test tomorrow.

I've already fixed the bitrot in bug 698630 locally.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27) > Does it speed up Santa's Workshop at all? Nope. No difference. I did some quick profiling of Santa's Workshop on Windows 7 with a nightly build and I get: d2d1.dll 32~34% of samples xul.dll 24~27% Within xul.dll 25~33% of samples occur within mozilla::SVGPathData::ConstructPath. The next in the list is nsIFrame::InvalidateInteral with about 5%. There's really not any serialisation going on in that test case so I guess this bug isn't going to help. It also doesn't seem to help with the helicopter test case for similar reasons.

Oh well. Thanks.

(In reply to Robert Longson from comment #31) > I've already fixed the bitrot in bug 698630 locally. Ok, in that case bug 696210 may as well land before this one since this one will probably take longer to review.

We either need to add an assignment operator to nsAttrValue (what this patch does) or disable it (i.e. make it private). Otherwise it's really easy to shoot yourself in the foot by assigning one nsAttrValue object--currently if you do that when the payload is a string the nsStringBuffer refcount won't get incremented and will therefore may eventually get deallocated while nsAttrValues still point to it.

This patch is somewhat questionable. A lot of the code in the previous patches deals with checking for and ignoring redundant changes via the DOM APIs. Previously SetParsedAttr used to do this for us by comparing strings before doing anything. Now that we're not serialising though it can't do that for us so we have to do it ourselves. In the patches up to this point we matched the previous behaviour by adding a lot of redundancy checking to the SVG types. This has the advantage of filtering out unnecessary changes as early as possible. However, I think the only place this redundancy checking (or lack thereof) is observable from the outside is via mutation events. So another option is to just check the string fields of the mutation event if and when we send one since in that case we will have to serialise the attribute values anyway. It means we don't filter out redundant changes until much later but it means less code. :) I'm quite ok with not including this patch. Let me know what you think.

There are actually two purposes to this patch: a) Stop exporting SVG types from the SVG module just for nsAttrValue's sake b) Tidy up nsAttrValue to remove redundant code when handling the SVG types. I think (a) is important. However, (b) is perhaps a little hacky in parts but not too bad. Depending on what you think we can alter this patch to just do (a).

That's it for now. I'll have a think about how to split up the review workload a bit since it's a bit rough on Jonathan to land him with 19!

What about Bug #698996 being a blocker? Does it belong here or somewhere else?

(In reply to Daniel Roesler from comment #61) > What about Bug #698996 being a blocker? Does it belong here or somewhere > else? Yeah, I just ran a quick test with that one now and, while I don't want to give hard numbers because there's currently a lot of noise on my system, these patches make a big difference. I did a quick profile of that test case too and a lot of the work is in serialising path data (when setting the path segments) which is where these patches help.

Try run for 65abe8c5de1f is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=65abe8c5de1f Results (out of 214 total builds): exception: 1 success: 185 warnings: 26 failure: 2 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-65abe8c5de1f

Try run for b769712f67f3 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=b769712f67f3 Results (out of 220 total builds): success: 184 warnings: 32 failure: 4 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-b769712f67f3

Bug fix.

Update roll-up patch with changes to part 2

Try run for 728a619377a7 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=728a619377a7 Results (out of 209 total builds): success: 189 warnings: 20 Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-728a619377a7

Comment on attachment 573114 [details] [diff] [review] Part 8 - Remove unnecessary serialisation from setting nsSVGLength2 >+// Helper methods for Will/DidChangeXXX method pairs. >+// >+// For some SVG data types, we use an nsAttrValue that just points to an SVG >+// object (e.g. nsSVGLength2). This saves storing the attribute value twice >+// (once here in SVG land and once more in the nsAttrValue). However, it means >+// that once we update the SVG object the old value is lost. >+// >+// In the case where we have mutation listeners, we need the old value in order >+// to set the prevValue member of the mutation event. In order to support this >+// we require a separate call to WillChangeXXX before changing the SVG class >+// where we pass back a serialized version of the (soon to be old) attribute >+// value. If there are no mutation listeners (the common case) we skip this >+// serialisation altogether and just return an empty attribute value. >+// >+// Note that unlike using SetParsedAttr, using Will/DidChangeXXX does NOT check >+// and filter out redundant changes. Before calling WillChangeXXX, the call site >+// should check if the value will change and if not, skip calling >+// Will/DidChangeXXX. >+nsAttrValue >+nsSVGElement::WillChangeValue(nsIAtom* aName) >+{ My understanding is that the code that checks whether an attribute change should trigger a restyle also needs the old attribute value in order to work. Boris should be able to explain that in more detail. Unfortunately, if correct, it would seem that when someone changes an SVG attribute's DOM representation we're either going to have to serialize or create a second internal object tree to represent the old value. Either way, it seems hard to avoid the perf hit. I'll attach a testcase for you to check whether your current patches break restyling.

> My understanding is that the code that checks whether an attribute change should trigger > a restyle also needs the old attribute value in order to work. Yes, though we could talk about trying to change that... > we're either going to have to serialize or create a second internal object tree to > represent the old value. Or call AttributeWillChange _before_ mutating the old value's internal data structure. That's what @style does, for example.

(In reply to Boris Zbarsky (:bz) from comment #70) > Or call AttributeWillChange _before_ mutating the old value's internal data > structure. That's what @style does, for example. Yeah, that's what these patches do too (see part 8 -- nsSVGElement::WillChangeValue). The test case seems to work in my patched build too.

I think my assumptions about what the restyling code does are wrong - I thought that it needs to be able to simultaneously have access to both the old value and the new value to check if the attribute has genuinely changed and, if so, do any necessary restyling. Given that assumption, it's not clear to me how calling anything earlier helps without some way of storing the old/new value separately so they can be compared. Can one of you clarify?

> I thought that it needs to be able to simultaneously have access to both the old value > and the new value Ah. No, it does not need that. It needs access to the old value in AttributeWillChange and access to the new value in AttributeChanged.

(In reply to Brian Birtles (:birtles) from comment #29) > (In reply to Robert Longson from comment #28) > > It looks like this depends on bug 696078 and bug 698195 There are overrides > > to DidChangeValue in nsSVGFilters that need to go away before you can make > > them non-virtual. bug 696210 is also in the same area and can only land > > after bug 696078 and bug 698195), maybe this bug superseeds that though. > > I think bug 696078 is only dealing with changes to DidAnimateXXX? So no > conflict? > > This patch doesn't touch DidChangeString--it leaves it as virtual so it > shouldn't be dependent on bug 698195. > > Having a look at bug 696210, that bug's patch and this work overlap > significantly. About half of the patch for bug 696210 is covered by this > patch (the missing code being mostly the changes to DidChangeString and > DidAnimateXXX). > We can either: > a) Land bug 696210 as is (after fixing bitrot from bug 698630) and rework > this bug to fit on top, or > b) Alter bug 696210 to just deal with the DidAnimateXXX changes, then deal > with DidChangeString in bug 698195 and the rest of the DidChangeXXX methods > here. That would save you having to fix most of the bitrot caused by bug > 698630. > I think b) is the way to go. Can you integrate the DidCahngeXXX parts of bug 696210 that make sense here? I'll rework what's left in bug 696210 later.

Fix bitrot.

Fix bitrot.

Fix bitrot.

(In reply to Robert Longson from comment #74) > I think b) is the way to go. Can you integrate the DidCahngeXXX parts of bug > 696210 that make sense here? I'll rework what's left in bug 696210 later. Done. Basically the only thing which was missing was a couple of tweaks to DidChangeString which I've added to patch 16 here. I've gone through all the other code in bug 696210 and it's pretty much all included in these patches which the exception of making the DidAnimateXXX stuff non-virtual.

Boris and Jonathan is there anything I can do to help with getting these patches reviewed? It's been nearly a month now and I was hoping to get this onto the next release train. Jonathan, I realise I've asked you to review a lot of patches. I didn't divide the work up further because those patches are all very similar. Once you've reviewed the first three patches or so, you can probably review the others very quickly. But if it still seems overwhelming perhaps I can ask Robert Longson to look at half of them?

Sorry Brian, I should have said, but I was waiting to see that Boris was happy with you contents changes before reviewing how you use those changes in SVG. Boris, do you think you'll able to look at this any time soon?

There's also bug 607854 which adds the one missing type from SVG - string lists We'll need to integrate this change with that one at some point.

> Boris, do you think you'll able to look at this any time soon? Hopefully toward the end of this week.....

bug 607854 has landed so can you convert the new StringList type Brian?

I'm sorry for the lag here; I've been spending way more time than I expected on some other things, and then holidays happened... :( I'm not likely to get to this before Tuesday. I do hope to look at it at that point, but if that doesn't happen it'll be end of January. If Jonas can review before those timeframes (which I sort of doubt), it might be worth asking him instead. Again, I'm really sorry for the lag.

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot

Fix bitrot