Redesign how we send change notifications from SVG types

RESOLVED FIXED in mozilla13

Status

()

defect
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: jwatt, Assigned: birtles)

Tracking

(Blocks 3 bugs, {perf})

Trunk
mozilla13
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(28 attachments, 116 obsolete attachments)

628 bytes, image/svg+xml
Details
1.97 KB, patch
bzbarsky
: review+
birtles
: checkin+
Details | Diff | Splinter Review
4.19 KB, text/plain
Details
3.63 KB, text/plain
Details
10.60 KB, patch
Details | Diff | Splinter Review
13.26 KB, patch
Details | Diff | Splinter Review
33.27 KB, patch
Details | Diff | Splinter Review
34.09 KB, patch
Details | Diff | Splinter Review
9.13 KB, patch
Details | Diff | Splinter Review
16.90 KB, patch
Details | Diff | Splinter Review
9.62 KB, patch
Details | Diff | Splinter Review
17.44 KB, patch
Details | Diff | Splinter Review
22.60 KB, patch
Details | Diff | Splinter Review
10.11 KB, patch
Details | Diff | Splinter Review
6.63 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
17.78 KB, patch
Details | Diff | Splinter Review
14.78 KB, patch
Details | Diff | Splinter Review
20.78 KB, patch
Details | Diff | Splinter Review
21.46 KB, patch
Details | Diff | Splinter Review
22.33 KB, patch
Details | Diff | Splinter Review
28.25 KB, patch
Details | Diff | Splinter Review
303.11 KB, patch
Details | Diff | Splinter Review
21.76 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
21.77 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
47.48 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
4.57 KB, patch
birtles
: checkin+
Details | Diff | Splinter Review
29.78 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
27.51 KB, patch
jwatt
: review+
Details | Diff | Splinter Review
Reporter

Description

9 years ago
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.
Reporter

Comment 1

9 years ago
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.
Reporter

Updated

9 years ago
Keywords: perf
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.
Version: unspecified → Trunk
Assignee

Comment 3

8 years ago
Taking this as it seems to be the root cause of bug 646510 which is assigned to me.
Assignee: nobody → birtles
Blocks: 646510
Status: NEW → ASSIGNED
Depends on: 602759
Reporter

Comment 4

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

Comment 5

8 years ago
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.
Assignee

Comment 7

8 years ago
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.
Assignee

Comment 9

8 years ago
Assignee

Comment 12

8 years ago
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.
Assignee

Comment 17

8 years ago
(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.
Assignee

Comment 18

8 years ago
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.
Assignee

Comment 19

8 years ago
Posted patch WIP patch 2 (obsolete) — Splinter Review
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.
Attachment #563626 - Attachment is obsolete: true
Attachment #563627 - Attachment is obsolete: true
Attachment #563628 - Attachment is obsolete: true
Attachment #563629 - Attachment is obsolete: true
Attachment #563630 - Attachment is obsolete: true
Attachment #563631 - Attachment is obsolete: true
Attachment #563632 - Attachment is obsolete: true
Attachment #563633 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #570943 - Attachment description: WIP patch 1 → WIP patch 2
do you need the aDoSetAttr argument in Will/DidChange. Can't you just not call the method at all if the argument is false?
Assignee

Comment 21

8 years ago
(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...
Assignee

Comment 24

8 years ago
(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.
Assignee

Comment 25

8 years ago
Posted patch WIP patch 3 (obsolete) — Splinter Review
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.
Attachment #570943 - Attachment is obsolete: true
Assignee

Comment 26

8 years ago
Posted patch WIP patch 4 (obsolete) — Splinter Review
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.
Attachment #571904 - Attachment is obsolete: true
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.
Assignee

Comment 29

8 years ago
(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?
Assignee

Comment 30

8 years ago
(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.
Assignee

Comment 32

8 years ago
(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.
Assignee

Comment 34

8 years ago
(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.
Assignee

Comment 36

8 years ago
Attachment #573108 - Flags: review?(bzbarsky)
Assignee

Comment 37

8 years ago
Attachment #573109 - Flags: review?(bzbarsky)
Assignee

Comment 38

8 years ago
Attachment #573110 - Flags: review?(bzbarsky)
Assignee

Comment 39

8 years ago
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.
Attachment #573111 - Flags: review?(bzbarsky)
Assignee

Comment 40

8 years ago
Attachment #573112 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573114 - Attachment is patch: true
Assignee

Comment 57

8 years ago
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.
Attachment #573131 - Flags: review?(jwatt)
Assignee

Comment 58

8 years ago
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).
Attachment #573132 - Flags: review?(jwatt)
Assignee

Comment 60

8 years ago
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!
Assignee

Updated

8 years ago
No longer blocks: 608495

Comment 61

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

Comment 62

8 years ago
(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
Assignee

Comment 65

8 years ago
Bug fix.
Attachment #573108 - Attachment is obsolete: true
Attachment #573743 - Flags: review?(bzbarsky)
Attachment #573108 - Flags: review?(bzbarsky)
Assignee

Comment 66

8 years ago
Update roll-up patch with changes to part 2
Attachment #573133 - Attachment is obsolete: true
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
Reporter

Comment 68

8 years ago
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.
Assignee

Comment 71

8 years ago
(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.
Reporter

Comment 72

8 years ago
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.
Assignee

Comment 75

8 years ago
Fix bitrot.
Attachment #573743 - Attachment is obsolete: true
Attachment #578986 - Flags: review?(bzbarsky)
Attachment #573743 - Flags: review?(bzbarsky)
Assignee

Comment 76

8 years ago
Fix bitrot.
Attachment #573109 - Attachment is obsolete: true
Attachment #578987 - Flags: review?(bzbarsky)
Attachment #573109 - Flags: review?(bzbarsky)
Assignee

Comment 77

8 years ago
Fix bitrot.
Attachment #573114 - Attachment is obsolete: true
Attachment #578988 - Flags: review?(jwatt)
Attachment #573114 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578989 - Flags: review? → review?(jwatt)
Assignee

Comment 79

8 years ago
Attachment #573123 - Attachment is obsolete: true
Attachment #578990 - Flags: review?(jwatt)
Attachment #573123 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578989 - Attachment description: Add mutation event tests for strings and classes and tidy up DidChangeString usage v2 → Part 16 Add mutation event tests for strings and classes and tidy up DidChangeString usage v2
Assignee

Updated

8 years ago
Attachment #578989 - Attachment description: Part 16 Add mutation event tests for strings and classes and tidy up DidChangeString usage v2 → Part 16 - Add mutation event tests for strings and classes and tidy up DidChangeString usage v2
Assignee

Comment 80

8 years ago
Attachment #573124 - Attachment is obsolete: true
Attachment #578991 - Flags: review?(jwatt)
Attachment #573124 - Flags: review?(jwatt)
Assignee

Comment 81

8 years ago
Attachment #573125 - Attachment is obsolete: true
Attachment #578992 - Flags: review?(jwatt)
Attachment #573125 - Flags: review?(jwatt)
Assignee

Comment 82

8 years ago
Attachment #573126 - Attachment is obsolete: true
Attachment #578993 - Flags: review?(jwatt)
Attachment #573126 - Flags: review?(jwatt)
Assignee

Comment 83

8 years ago
Attachment #573128 - Attachment is obsolete: true
Attachment #578994 - Flags: review?(jwatt)
Attachment #573128 - Flags: review?(jwatt)
Assignee

Comment 84

8 years ago
Attachment #573129 - Attachment is obsolete: true
Attachment #578995 - Flags: review?(jwatt)
Attachment #573129 - Flags: review?(jwatt)
Assignee

Comment 85

8 years ago
(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.
Assignee

Comment 86

8 years ago
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?
Assignee

Comment 87

8 years ago
Attachment #573744 - Attachment is obsolete: true
Reporter

Comment 88

8 years ago
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.
Assignee

Updated

8 years ago
Attachment #573107 - Attachment is obsolete: true
Attachment #573107 - Flags: review?(bzbarsky)
Assignee

Comment 94

8 years ago
Fix bitrot
Attachment #592564 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #578986 - Attachment is obsolete: true
Attachment #578986 - Flags: review?(bzbarsky)
Assignee

Comment 95

8 years ago
Fix bitrot
Attachment #592566 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #578987 - Attachment is obsolete: true
Attachment #578987 - Flags: review?(bzbarsky)
Assignee

Comment 96

8 years ago
Fix bitrot
Attachment #592567 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #573110 - Attachment is obsolete: true
Attachment #573110 - Flags: review?(bzbarsky)
Assignee

Comment 97

8 years ago
Fix bitrot
Attachment #592568 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #573111 - Attachment is obsolete: true
Attachment #573111 - Flags: review?(bzbarsky)
Assignee

Comment 98

8 years ago
Fix bitrot
Attachment #592569 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573112 - Attachment is obsolete: true
Attachment #573112 - Flags: review?(jwatt)
Assignee

Comment 99

8 years ago
Fix bitrot
Attachment #592570 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573113 - Attachment is obsolete: true
Attachment #573113 - Flags: review?(jwatt)
Assignee

Comment 100

8 years ago
Fix bitrot
Attachment #592571 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578988 - Attachment is obsolete: true
Attachment #578988 - Flags: review?(jwatt)
Assignee

Comment 101

8 years ago
Fix bitrot
Attachment #592572 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573115 - Attachment is obsolete: true
Attachment #573115 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573116 - Attachment is obsolete: true
Attachment #573116 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573117 - Attachment is obsolete: true
Attachment #573117 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573118 - Attachment is obsolete: true
Attachment #573118 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573119 - Attachment is obsolete: true
Attachment #573119 - Flags: review?(jwatt)
Assignee

Comment 106

8 years ago
Fix bitrot
Attachment #592578 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573120 - Attachment is obsolete: true
Attachment #573120 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573121 - Attachment is obsolete: true
Attachment #573121 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578989 - Attachment is obsolete: true
Attachment #578989 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578990 - Attachment is obsolete: true
Attachment #578990 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578991 - Attachment is obsolete: true
Attachment #578991 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578992 - Attachment is obsolete: true
Attachment #578992 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578993 - Attachment is obsolete: true
Attachment #578993 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578994 - Attachment is obsolete: true
Attachment #578994 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #578995 - Attachment is obsolete: true
Attachment #578995 - Flags: review?(jwatt)
Assignee

Comment 115

8 years ago
Fix bitrot
Attachment #592587 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573131 - Attachment is obsolete: true
Attachment #573131 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #573132 - Attachment is obsolete: true
Attachment #573132 - Flags: review?(jwatt)
Assignee

Comment 117

8 years ago
Patches should now be up to date. I've yet to convert StringList as suggested in comment 91 but that patch should just sit on top of the others.

Next week it will be 3 months since I first requested review. I'd like to get the review process moving since this is an important perf win. Boris, perhaps you could suggest someone else who is suitable for reviewing this? Thanks!
For this code it's probably me or sicking.

For what it's worth, I was planning to make some progress on this review this week.  I just needed to free up a few days in a row to work on it...
Assignee

Comment 119

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #118)
> For this code it's probably me or sicking.
> 
> For what it's worth, I was planning to make some progress on this review
> this week.  I just needed to free up a few days in a row to work on it...

Ok, that'd be great. Thanks Boris!
Assignee

Comment 120

8 years ago
Remove forward declarations from nsAttrValue.h and just replace with SVGAttrValueWrapper.h which contains them anyway
Attachment #592607 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #592588 - Attachment is obsolete: true
Attachment #592588 - Flags: review?(jwatt)
Assignee

Comment 121

8 years ago
Incorporate SVGStringList too
Attachment #592609 - Flags: review?(jwatt)
Try run for e96c921bf6d9 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=e96c921bf6d9
Results (out of 208 total builds):
    success: 188
    warnings: 20
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-e96c921bf6d9
Assignee

Comment 123

8 years ago
Comment on attachment 592609 [details] [diff] [review]
part 25 - Remove unnecessary serialisation from setting SVGStringList

Forgot to include serialisation. Updated patch forthcoming.
Attachment #592609 - Attachment is obsolete: true
Attachment #592609 - Flags: review?(jwatt)
Assignee

Comment 124

8 years ago
Updated SVGStringList patch. I had to move the "isCommaSeparated" flag into the SVGStringList class since nsAttrValue objects need to know how to serialise themselves.
Attachment #592619 - Flags: review?(jwatt)
Assignee

Comment 125

8 years ago
Fix bitrot
Attachment #592951 - Flags: review?(bzbarsky)
Assignee

Updated

8 years ago
Attachment #592567 - Attachment is obsolete: true
Attachment #592567 - Flags: review?(bzbarsky)
Reporter

Comment 126

8 years ago
Comment on attachment 592570 [details] [diff] [review]
part 7 - Remove unnecessary serialisation from setting SVGEnum

> nsSVGElement::ParseAttribute(PRInt32 aNamespaceID,
>                              nsIAtom* aAttribute,
>                              const nsAString& aValue,
>                              nsAttrValue& aResult)
> {
>   nsresult rv = NS_OK;
>   bool foundMatch = false;
>+  bool setResult = false;

This sounds like the imperative. Can you change it to didSetResult? As an extra patch tacked onto the end of your patch queue is fine, so you don't need to juggle your patches.


>+nsIAtom*
>+nsSVGEnum::GetBaseValueAtom(nsSVGElement *aSVGElement)
> {
...
>   }
>   NS_ERROR("unknown enumeration value");
>+  return nsnull;

Seems safer to return nsGkAtoms::_empty here.
Attachment #592570 - Flags: review?(jwatt) → review+
Reporter

Updated

8 years ago
Duplicate of this bug: 609527
Comment on attachment 592563 [details] [diff] [review]
part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings

>+nsGenericElement::MaybeCheckSameAttrVal(PRInt32 aNamespaceID,
>+        // Need to store the old value.
>+        // If the current attribute value contains a pointer to some other data
>+        // structure that gets updated in the process of setting the attribute
>+        // we'll no longer have the old value of attribute.

"of the attribute"

>+        if (info.mValue->Type() != nsAttrValue::eString &&
>+            info.mValue->Type() != nsAttrValue::eAtom) {
>+          nsAutoString serializedValue;
>+          info.mValue->ToString(serializedValue);
>+          aOldValue.SetTo(serializedValue);
>+        } else {
>+          aOldValue.SetTo(*info.mValue);
>+        }

Hmm.  This is a bit of a pain, but I guess we're not going to need this anywhere else so there's no point moving it into an nsAttrValue method?

>+      // Compare the values but remember that both aValue (the new value) and
>+      // the existing value may a parsed value or a regular string value.

"may be a parsed value"

>+      // First, try a typed comparison:

This part I _definitely_ think we should move into nsAttrValue.  Maybe call it LooselyEquals or EqualsAsStrings or something?  There's the confusion that we have both *info.mValue and aOldValue, I guess...

In any case, this should be factored out as a separate method.  The code as written is _really_ hard to follow, and seems to be relying on various nsAttrValue internal details.  The weird indentation on the comments doesn't help....

> nsGenericElement::SetAttrAndNotify(PRInt32 aNamespaceID,
>+    if (!aOldValue.IsEmptyString()) {
>+      switch (aOldValue.Type()) {
....

This whole chunk with the switch and such should become an |already_AddRefed<nsIAtom> nsAttrValue::GetAsAtom() const| method.  That way we don't have to worry about attr value guts here.

>+++ b/content/base/src/nsGenericElement.h
>   bool MaybeCheckSameAttrVal(PRInt32 aNamespaceID, nsIAtom* aName,

aOldValue is always set as a string, right?  Will that continue being the case?  Might be worth documenting exactly what one can expect or not out of aOldValue.

I'd like to see an updated version of this patch with the refactoring bits, but modulo the nits above this looks good.
Attachment #592563 - Flags: review?(bzbarsky) → review+
Comment on attachment 592564 [details] [diff] [review]
part 2 - Make BeforeSetAttr take an nsAttrValue

r=me
Attachment #592564 - Flags: review?(bzbarsky) → review+
Comment on attachment 592566 [details] [diff] [review]
part 3 - Make AfterSetAttr take an nsAttrValue

In nsGenericHTMLFormElement::AfterSetAttr for the name/id case we know we have an atom value, right?  You should be able to use nsDependentAtomString instead of having to ToString the value, I think.

For the nsXULElement::AfterSetAttr case, we only have to check Type() because it can be an atom, right?  nsAttrValue could easily return an nsCheapString in the atom case too.  Could you please file a followup bug on adding a way to do that and making the XUL code use that?

>+              HideWindowChrome(aValue->Equals(NS_LITERAL_STRING("true"),
>+                               eCaseMatters));

Weird indent here.  Please fix.

r=me with that
Attachment #592566 - Flags: review?(bzbarsky) → review+
Comment on attachment 592951 [details] [diff] [review]
part 4 - Tidy up bool parameters to make them easier to read

r=me
Attachment #592951 - Flags: review?(bzbarsky) → review+
Comment on attachment 592568 [details] [diff] [review]
part 5 - Add assignment operator to nsAttrValue

r=me

I'm really sorry this took so long.  I recall starting to read this patch queue and feeling like it was really scary and complicated, but this time around it didn't seem that bad.... :(

Next time please poke me earlier?  ;)
Attachment #592568 - Flags: review?(bzbarsky) → review+
Assignee

Updated

8 years ago
Attachment #592563 - Attachment is obsolete: true
Assignee

Updated

8 years ago
Attachment #594643 - Attachment description: part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; → part 1 - Make MaybeCheckSameAttrVal work on parsed attributes instead of strings; r=bz
Attachment #594643 - Flags: review+
Assignee

Comment 134

8 years ago
Address review feedback.
Attachment #592566 - Attachment is obsolete: true
Attachment #594644 - Flags: review+
Assignee

Updated

8 years ago
Blocks: 724466
Assignee

Comment 135

8 years ago
Address review feedback.
Attachment #592570 - Attachment is obsolete: true
Attachment #594645 - Flags: review+
Assignee

Comment 136

8 years ago
Updated roll-up patch for cross reference.
Attachment #578998 - Attachment is obsolete: true
Assignee

Comment 137

8 years ago
Thanks Boris! Much appreciated. The first patch looks a lot better with your feedback incorporated. Please give it a glance and check that it matches what you had in mind.

Thanks Jonathan as well! I went ahead and updated the patches directly instead of adding another patch on top since I was touching them all anyway to rebase on top of the changes from Boris' reviews. To avoid bug spam, I haven't bothered posting all the updated patches however--I'll wait until I get your feedback before updating them here. In the mean time, that means the current queue won't compile but I've posted an updated roll-up patch that should.

I'm going to go ahead and send patches 1-5 to Try. Depending on how long you think it will take you Jonathan, it might be worth landing those patches first to avoid more bitrot.
Assignee

Comment 138

8 years ago
Fix missing call to set the attribute using the parsed attribute interface.
Attachment #592619 - Attachment is obsolete: true
Attachment #594650 - Flags: review?(jwatt)
Attachment #592619 - Flags: review?(jwatt)
Try run for d5f951fa96a5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d5f951fa96a5
Results (out of 273 total builds):
    success: 204
    warnings: 48
    failure: 21
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-d5f951fa96a5
Reporter

Comment 140

8 years ago
(In reply to Brian Birtles (:birtles) from comment #137)
> it might be worth landing those patches first to avoid more bitrot.

Please do, although I'm going to be working on getting through these patches as soon as possible.
Reporter

Comment 141

8 years ago
My main concern about these patches is whether it's safe for nsAttrValue objects to have raw, non-owning pointers into SVG element objects. It's not entirely clear to me whether element objects always outlive all nsAttrValue objects created for them.

Boris, presumably you considered that during your reviews and you're fine with that?
> Boris, presumably you considered that during your reviews and you're fine with that?

I didn't consider it, since the patches I looked at didn't do anything like that.

That said, I believe this is safe.  nsAttrValue objects can basically live in three places:

1)  On the stack.  This should be safe, right?
2)  In the element's attr and child array (as values, not as pointers!).  This is
    obviously safe if the element keeps the things being pointed to alive.
3)  In the mapped attributes.  As long as none of the relevant SVG attributes are mapped,
    this is a non-issue.  That should be double-checked, and perhaps asserted somewhere?
    Maybe in whatever ParseAttribute functions create such attr values?

> Please give it a glance and check that it matches what you had in mind.

Looks pretty good.  Please document the new nsAttrValue methods, though.

Also, in part 3 in nsGenericHTMLFormElement::AfterSetAttr the NS_ABORT_IF_FALSE seems to be backwards, no?  The value should be an atom value there...
> Aren't outer svg element width and height values mapped

Yes, they are.  OK, then we have to think about it a bit more.

So in general mapped attributes are stored in a refcounted nsMappedAttributes data structure.  The nsAttrAndChildArray holds a reference to this object.  These objects _can_ be shared across multiple attr and child arrays.  The way this works is that whenever an nsMappedAttributes is modified we take the post-modification version and see whether we have an existing object equal to it (in the nsMappedAttributes::Equals sense).  If so, we drop the just-modified object and point to the existing one.  Note that this involves Equals() testing true on the corresponding nsAttrValues in the two lists.  So as long as nsAttrValue::Equals uses pointer equality for the SVG stuff there should be no cross-element coalescing of mapped attributes that hold the SVG objects, and then the element's nsAttrAndChildArray will hold the unique ref to the nsMappedAttributes and everything should be fine.  Does mean the assert suggestion won't work. ;)
(In reply to Boris Zbarsky (:bz) from comment #144)
> nsMappedAttributes and everything should be fine.  Does mean the assert
> suggestion won't work. ;)

Yes, I don't think it will work unless you do something special for those two attributes.
Reporter

Updated

8 years ago
Duplicate of this bug: 340835
Reporter

Comment 147

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #144)
> So in general mapped attributes are stored in a refcounted
> nsMappedAttributes data structure.

Just a note - that's not the case currently for the SVG code (we don't use nsMappedAttributes), but it very possibly will be at some point in the future (if we push the mapped attribute stuff from nsGenericHTMLElement up to nsGenericElement). The patches here should be written to assume that will happen, I think (in terms of adding asserts).

> The nsAttrAndChildArray holds a
> reference to this object.  These objects _can_ be shared across multiple
> attr and child arrays.

This sharing happens as a result of the nsAttrAndChildArray::MakeMappedUnique() calls, right? Probably we should not call that for SVG elements if/when we push the mapped attr stuff up to nsGenericElement...?

> as long as
> nsAttrValue::Equals uses pointer equality for the SVG stuff

Hmm, it doesn't seem like we should do pointer comparison there, except as a return-true shortcut. That was actually one of my upcoming comments on this patch series.

> Does mean the assert suggestion won't work.

I think we should have asserts in an inline nsSVGElement::AfterSetAttr to check |nsAttrAndChildArray::MappedAttrCount() == 0| for the moment, then if we move the mapped attr stuff up to nsGenericElement that asserts will remind us to make sure nsMappedAttributes objects are not shared between SVG elements.
> we don't use nsMappedAttributes

You don't?  You're returning true from IsAttributeMapped(), no?  nsGenericElement uses nsMappedAttributes in that case.  Or am I missing something.

> This sharing happens as a result of the nsAttrAndChildArray::MakeMappedUnique() calls,
> right?

Yes.  Skipping calling it might make sense if it'll always be a no-op for SVG.  But then why would you want to use nsMappedAttributes at all?  The whole point of that is sharing across elements; if you don't plan to use it, you could just use some other simple implementation for SVG.

> Hmm, it doesn't seem like we should do pointer comparison there

OK.  Then we need to be very careful about mapped attributes.

> I think we should have asserts in an inline nsSVGElement::AfterSetAttr to check
> |nsAttrAndChildArray::MappedAttrCount() == 0| for the moment

Does such an assert not assert on trunk right now?
Reporter

Comment 149

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #148)
> > we don't use nsMappedAttributes
> 
> You don't?  You're returning true from IsAttributeMapped(), no? 
> nsGenericElement uses nsMappedAttributes in that case.  Or am I missing
> something.

We return true from IsAttributeMapped, but SVG elements don't inherit nsMappedAttributeElement, so the SetMappedAttribute call invokes nsGenericElement::SetMappedAttribute which returns false.

> > This sharing happens as a result of the nsAttrAndChildArray::MakeMappedUnique() calls,
> > right?
> 
> Yes.  Skipping calling it might make sense if it'll always be a no-op for
> SVG.  But then why would you want to use nsMappedAttributes at all?

So that the SVG code can share the code that gets the nsIStyleRule from nsMappedAttributes, and use the MapRuleInfoInto from that, rather than having it's own custom code for mapping attributes into style?

> The
> whole point of that is sharing across elements; if you don't plan to use it,
> you could just use some other simple implementation for SVG.

Ok, maybe we can just keep the SVG implementation if it's not really a win to share the normal attr mapping code.

> > Hmm, it doesn't seem like we should do pointer comparison there
> 
> OK.  Then we need to be very careful about mapped attributes.

Yup. Would the following assert do...?

> > I think we should have asserts in an inline nsSVGElement::AfterSetAttr to check
> > |nsAttrAndChildArray::MappedAttrCount() == 0| for the moment
> 
> Does such an assert not assert on trunk right now?

Not that I'm aware of.
Reporter

Comment 150

8 years ago
Of course nsAttrAndChildArray::MappedAttrCount() is private - is it ok to make that public, bz?
> but SVG elements don't inherit nsMappedAttributeElement, so the SetMappedAttribute call
> invokes nsGenericElement::SetMappedAttribute

Ah, right.  We have too many element classes.  :(

> So that the SVG code can share the code that gets the nsIStyleRule from
> nsMappedAttributes, and use the MapRuleInfoInto from that, rather than having it's own
> custom code for mapping attributes into style?

OK... I'd have to dig through a bit to see whether you'd really be sharing all that much code once you're not sharing a bunch of the real guts (which is the coalescing across elements).  In particular, if the element could have a direct pointer to the style rule (which sounds like what you'd be doing in the SVG case), then bloating the mapped attribute hashtable with all those singleton rules may not be a very good idea.  I agree that we'd want to restructure the code somehow to make you not have to reinvent wheels, though.

> Yup. Would the following assert do...?

Yes.  Though make it HasMappedAttributes() for the public method, since that's what you really want to know.
> > The
> > whole point of that is sharing across elements; if you don't plan to use it,
> > you could just use some other simple implementation for SVG.
> 
> Ok, maybe we can just keep the SVG implementation if it's not really a win
> to share the normal attr mapping code.

To be clear, the main point of the sharing is so that all elements with the same set of style mapping attributes share the same nsIStyleRule object.

This is important due to some details of how the style-rule-tree works since otherwise you can end up with an explosion of style-rule-tree branches due to the fact that the nsIStyleRules for mapped attributes have very low precedence.

At least that is my understanding. Though that might have changed over the years and is no longer the case, or is no longer as important.
It's still important for HTML (think a page with a bajillion of <font> tags with the same "face" attribute).  The key questions are:

1)  Would this sort of sharing be useful for SVG?  That is would it be common to have lots of elements with identical style-mapped attributes on them?  I'd guess no.

2)  Given that the sharing is not useful for SVG, how could/should we refactor the mapped attr code so that we have minimal code duplication but don't do all the attempted sharing stuff, sorting, storage in a separate object with resulting mis-ordering of attributes, etc, for SVG?
Reporter

Comment 154

8 years ago
I've spun off bug 724680 to discuss sharing more of the mapped attribute code to keep down the noise here. Can you add your comments there?
Reporter

Comment 155

8 years ago
Brian, I think for you the only actionable point since comment 141 is:

Please add a public HasMappedAttributes() method to nsAttrAndChildArray, then add an inline nsSVGElement::AfterSetAttr with an NS_ABORT_IF_FALSE that checks that it always returns false for SVG elements. Also add a nice detailed comment there explaining why we need to be very careful if we start using nsMappedAttributes for SVG elements (i.e. some nsAttrValues point to member data of SVG elements, and we don't want to leave dangling pointers). Probably have the comment point to bug 724680 too.
Reporter

Comment 156

8 years ago
(In reply to Jonathan Watt [:jwatt] from comment #155)
> then add an inline nsSVGElement::AfterSetAttr with...

Sorry, it's BeforeSetAttr that doesn't currently exist - obviously just add the assert and comment to the existing nsSVGElement::AfterSetAttr.
Reporter

Comment 157

8 years ago
Comment on attachment 592571 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

Review of attachment 592571 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsAttrValue.cpp
@@ +709,5 @@
>        return thisCont->mIntMargin == otherCont->mIntMargin;
>      }
> +    case eSVGLength:
> +    {
> +      return thisCont->mSVGLength == otherCont->mSVGLength;

We should be doing object comparison here, not pointer comparison. And we want to only compare the baseVal members. You could add a BaseValEquals() method to nsSVGLength2 (and the other types in the patch series) just for that purpose (rather than operator==).

That said, it would still be good to have a pointer comparison short circuit to save us having to compare objects (especially some of the larger object types such as lists) if the two objects are the same object. That check would probably be best off in the BaseValEquals() (or whatever you call it) method though, rather than here.

::: content/svg/content/src/nsSVGElement.cpp
@@ +1256,5 @@
>                                               SMIL_MAPPED_ATTR_STYLERULE_ATOM,
>                                               nsnull));
>  }
>  
> +// Helper methods for Will/DidChangeXXX method pairs.

Can you use a Doxygen style comment, and make the comment:

/**
 * Helper methods for the type-specific WillChangeXXX methods.
 *
 * This method sends out appropriate pre-change notifications so that selector
 * restyles (e.g. due to changes that cause |elem[attr="val"]| to start/stop
 * matching) work, and it returns an nsAttrValue that _may_ contain the
 * attribute's pre-change value.
 *
 * The nsAttrValue returned by this method depends on whether there are
 * mutation event listeners listening for changes to this element's attributes.
 * If not, then the object returned in empty. If there are, then the
 * nsAttrValue returned contains a serialized copy of the attribute's value
 * prior to the change, and this object _MUST_ be passed to the corresponding
 * DidChangeXXX method call. This is necessary so that the 'prevValue' property
 * of the mutation event that is dispatched will correctly contain the old
 * value.
 *
 * The reason we need to serialize the old value if there are mutation
 * event listeners is because the underlying nsAttrValue for the attribute
 * points directly to a parsed representation of the attribute (e.g. an
 * SVGAnimatedLengthList*) that is a member of the SVG element. That object
 * will have changed by the time DidChangeXXX has been called, so without the
 * serialization of the old attribute value that we provide, DidChangeXXX
 * would have no way to get the old value to pass to SetAttrAndNotify.
 *
 * We only return the old value when there are mutation event listeners because
 * it's not needed otherwise, and because it's expensive to serialize the old
 * value. This is especially true for list type attributes, which may be built
 * up via the SVG DOM resulting in a large number of Will/DidModifyXXX calls
 * before the script finally finishes setting the attribute.
 *
 * Note that unlike using SetParsedAttr, using Will/DidChangeXXX does NOT check
 * and filter out redundant changes. Before calling WillChangeXXX, the caller
 * should check whether the new and old values are actually the same, and skip
 * calling Will/DidChangeXXX if they are.
 */

@@ +1289,5 @@
> +
> +  // This is not strictly correct--the attribute value parameter for
> +  // BeforeSetAttr should reflect the value that *will* be set but that implies
> +  // allocating, e.g. an extra nsSVGLength2, and isn't necessary at the moment
> +  // since no SVG elements overload BeforeSetAttr. For now we just pass the

Probably worth adding a debug only BeforeSetAttr to nsSVGElement that defers to nsSVGElementBase, and is marked using MOZ_FINAL, with a comment flagging this issue.

@@ +1320,5 @@
> +  return emptyOrOldAttrValue;
> +}
> +
> +void
> +nsSVGElement::DidChangeValue(nsIAtom* aName, const nsAttrValue& aOldValue,

It seems like aOldValue should be aEmptyOrOldValue.

Again, a Doxygen style comment would be good:

/**
 * Helper methods for the type-specific DidChangeXXX methods.
 *
 * aEmptyOrOldValue must be the object returned from the corresponding
 * WillChangeXXX call. If we have mutation event listeners things will go
 * wrong if it isn't.
 */

@@ +1322,5 @@
> +
> +void
> +nsSVGElement::DidChangeValue(nsIAtom* aName, const nsAttrValue& aOldValue,
> +                             nsAttrValue& aNewValue)
> +{

It would be good if we could have an NS_ABORT_IF_FALSE below checking that if we have mutation listeners, aEmptyOrOldValue is not uninitialized. I'm not really sure how you can do that currently though, since the best nsAttrValue seems to offer is IsEmptyString(), which is not what we want. Maybe we can add a bogus "not initialized" type to nsAttrValue purely for this purpose? Boris/Jonas?

@@ +1440,5 @@
> +  return WillChangeValue(*GetLengthInfo().mLengthInfo[aAttrEnum].mName);
> +}
> +
> +void
> +nsSVGElement::DidChangeLength(PRUint8 aAttrEnum, const nsAttrValue& aOldValue)

And again here - aEmptyOrOldValue. And in fact for all DidChangeXXX definitions/declarations.

::: content/svg/content/src/nsSVGLength2.cpp
@@ +313,5 @@
> +  if (mIsBaseSet && mBaseVal == aValue) {
> +    return;
> +  }
> +
> +  nsAttrValue oldValue;

Can you rename oldValue to emptyOrOldValue, since it's only the old value if we have mutation listeners, right? Same goes for the other places where WillChangeLength (and indeed all other WillChangeXXX methods) are called.

@@ +338,5 @@
>    if (!IsValidUnitType(unitType))
>      return NS_ERROR_DOM_NOT_SUPPORTED_ERR;
>  
> +  if (mSpecifiedUnitType == PRUint8(unitType))
> +    return NS_OK;

I think we want to check mIsBaseSet here too, and send out notifications if it's false. If the default value of the length is 100%, say, and then someone calls convertToSpecifiedUnits() to set it to percentage units, then it seems that the attribute value should go from "" to "100%", even if there's no change to the underlying value. If that's the case, then we should send a mutation event. Probably we need a comment here to explain this as well.

@@ +340,5 @@
>  
> +  if (mSpecifiedUnitType == PRUint8(unitType))
> +    return NS_OK;
> +
> +  nsAttrValue oldValue = aSVGElement->WillChangeLength(mAttrEnum);

Before this line, can you add:

  // Even though we're not changing the visual effect this length will have
  // on the document, we still need to send out notifications in case we have
  // mutation listeners, since the actual string value of the attribute will
  // change.

@@ +346,4 @@
>    float valueInUserUnits = 
>      mBaseVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType);
>    mSpecifiedUnitType = PRUint8(unitType);
> +  SetBaseValue(valueInUserUnits, aSVGElement, false);

Hmm, under SetBaseValue() we call SetBaseValueInSpecifiedUnits(), so we'll get two Will/DidModifyLength calls for each ConvertToSpecifiedUnits() call...
Reporter

Comment 158

8 years ago
Comment on attachment 592587 [details] [diff] [review]
part 23 - Move redundancy checking from SVG types to event dispatch

I'm not sure we want to slow down that path for non-SVG elements.
Attachment #592587 - Flags: review?(bzbarsky)
Reporter

Comment 159

8 years ago
Plus we skip more work if we keep the redundancy checks in the SVG code.
The single atom compare on what's already a slow path is not a big deal for the non-SVG case.  So I'd just worry about how the SVG code wants stuff to work.
Attachment #592587 - Flags: review?(bzbarsky) → review+
Assignee

Updated

8 years ago
Whiteboard: [inbound, parts 1 to 5 only, don't close yet please]
Assignee

Updated

8 years ago
Attachment #592564 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #592568 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #594643 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #594644 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #592951 - Flags: checkin+
Assignee

Comment 162

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #142)
> Looks pretty good.  Please document the new nsAttrValue methods, though.

Oops, I missed this comment. Will post a follow-up patch soon.

> Also, in part 3 in nsGenericHTMLFormElement::AfterSetAttr the
> NS_ABORT_IF_FALSE seems to be backwards, no?  The value should be an atom
> value there...

That would explain why the previous try run was so colourful. :) Fixed in the version pushed to m-i.
Assignee

Comment 163

8 years ago
(In reply to Jonathan Watt [:jwatt] from comment #157)
> Comment on attachment 592571 [details] [diff] [review]
> part 8 - Remove unnecessary serialisation from setting nsSVGLength2
> 
> Review of attachment 592571 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/base/src/nsAttrValue.cpp
> @@ +709,5 @@
> >        return thisCont->mIntMargin == otherCont->mIntMargin;
> >      }
> > +    case eSVGLength:
> > +    {
> > +      return thisCont->mSVGLength == otherCont->mSVGLength;
> 
> We should be doing object comparison here, not pointer comparison. And we
> want to only compare the baseVal members. You could add a BaseValEquals()
> method to nsSVGLength2 (and the other types in the patch series) just for
> that purpose (rather than operator==).

Just out of curiousity why do we want to do object comparison?

Anyway, my guess is we're not using this for SVG types anyway. I added a breakpoint there and ran the SVG reftests and mochitests and it never got it. I'm now running the same assertion through Try to see if it picks up anything: https://tbpl.mozilla.org/?tree=Try&rev=a9e7362913ef

(By the way, do we have any tools for finding call sites--like Eclipse does with Java. MXR is not particularly helpful for searching for call sites of "Equals".)

Here is an updated patch which replaces the pointer comparison with an NS_ABORT_IF_FALSE(false). I'm reluctant to write all those comparison functions if I can't test them (and it's not just lengths we need to compare, but lists too, and lists of complex stuff--it could end up being quite a bit of code).
Attachment #592607 - Attachment is obsolete: true
Attachment #594927 - Flags: review?
Attachment #592607 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Attachment #594927 - Flags: review? → review?(jwatt)
Try run for 5e9beb968c53 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5e9beb968c53
Results (out of 272 total builds):
    success: 251
    warnings: 20
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bbirtles@mozilla.com-5e9beb968c53
Reporter

Comment 165

8 years ago
(In reply to Brian Birtles (:birtles) from comment #163)
> Just out of curiousity why do we want to do object comparison?

Because nsAttrValue::Equals() is supposed to check if two attributes are equal.

> Anyway, my guess is we're not using this for SVG types anyway.

Maybe not, but it seems like the code should still be correct so that if we in future things work as expected.

> (By the way, do we have any tools for finding call sites--like Eclipse does
> with Java. MXR is not particularly helpful for searching for call sites of
> "Equals".)

DXR is supposed to let you do that, but it seems to be broken right now:

http://dxr.lanedo.com/search.cgi?tree=mozilla-central&callers=nsAttrValue%3A%3AEquals

> Here is an updated patch which replaces the pointer comparison with an
> NS_ABORT_IF_FALSE(false). I'm reluctant to write all those comparison
> functions if I can't test them (and it's not just lengths we need to
> compare, but lists too, and lists of complex stuff--it could end up being
> quite a bit of code).

I added operator== methods to SVGLengthList et. al., so you should only need a small inline function on SVGAnimatedLengthList et. al. that uses that on the base vals of the two objects. That said, if you don't want to write that code please file a followup bug and assign it to me.
Reporter

Comment 166

8 years ago
Comment on attachment 594927 [details] [diff] [review]
part 24 - Add wrapper for SVG types used in nsAttrValue so we don't need to export them, v1b

Review of attachment 594927 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsAttrValue.cpp
@@ +888,5 @@
> +      if (IsSVGType(thisCont->mType)) {
> +        // Currently this method is never called for nsAttrValue objects that
> +        // point to SVG data types.
> +        // If that changes then we probably want to add methods to the
> +        // corresponding SVG types to compare their base values.

Please also note that we can compare pointers as a shortcut, just so we don't forget to do that.

::: content/base/src/nsAttrValue.h
@@ +128,5 @@
>      ,eAtomArray =      0x11
>      ,eDoubleValue  =   0x12
>      ,eIntMarginValue = 0x13
>      ,eSVGAngle =       0x14
> +    ,eSVGTypesBegin =  eSVGAngle

Please switch eSVGAngle and eSVGTypesBegin so that all the SVG types are bounded by eSVGTypesBegin and eSVGTypesEnd.

@@ +545,5 @@
> +    // All SVG types are just pointers to classes so just setting any of them
> +    // will do. We'll lose type-safety but the signature of the calling
> +    // function should ensure we don't get anything unexpected, and once we
> +    // stick aValue in a union we lose type information anyway.
> +    cont->mSVGAngle = (const nsSVGAngle*)&aValue;

If you're going to make use of this assumption, it seems like you might as well change |const T& aValue| to |const void *aValue| and avoid having the compiler create code for multiple templates.

Please use const_cast and static_cast (or reinterpret_cast if you decide not to do the |const void *aValue| thing) rather than the C-style cast.

@@ +547,5 @@
> +    // function should ensure we don't get anything unexpected, and once we
> +    // stick aValue in a union we lose type information anyway.
> +    cont->mSVGAngle = (const nsSVGAngle*)&aValue;
> +    cont->mType = aType;
> +    if (aSerialized && aSerialized->Length()) {

I think we should just call SetMiscAtomOrString without a check. I *hope* all the SVG types check to see if their baseVal is set when asked for their value as a string, but if any of them were to forget, we'd end up producing non-empty attribute values for default values.

Plus this is really an optimization for attr="" (attribute set to empty string), which doesn't seem worth it to me.
Attachment #594927 - Flags: review?(jwatt) → review+
Reporter

Comment 167

8 years ago
Comment on attachment 592571 [details] [diff] [review]
part 8 - Remove unnecessary serialisation from setting nsSVGLength2

I'll wait for a revised 'part 8' addressing the comments in comment 157 (except the "We should be doing object comparison here" comment).

I'll also proceed on the assumption that you will be updating the other patches for which comment 157 comments apply - please let me know if you don't plan to do that.
Attachment #592571 - Flags: review?(jwatt) → review-
Reporter

Comment 168

8 years ago
Comment on attachment 592572 [details] [diff] [review]
part 9 - Update attribute setting for SVGAnimatedLengthList

Review of attachment 592572 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGLength.cpp
@@ +168,5 @@
>        }
>        return NS_OK;
>      }
>    } else if (mUnit == nsIDOMSVGLength::SVG_LENGTHTYPE_NUMBER ||
>               mUnit == nsIDOMSVGLength::SVG_LENGTHTYPE_PX) {

Err, this branch should also be calling DidChangeLengthList - can you fix that while you're touching this code?
Attachment #592572 - Flags: review?(jwatt) → review+
Reporter

Comment 169

8 years ago
Comment on attachment 592573 [details] [diff] [review]
part 10 - Remove unnecessary serialisation from setting nsSVGNumber2

Review of attachment 592573 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGNumber2.cpp
@@ +152,5 @@
> +nsSVGNumber2::SetBaseValue(float aValue, nsSVGElement *aSVGElement)
> +{
> +  if (mIsBaseSet && aValue == mBaseVal) {
> +    return;
> +  }

Hmm, I think we still need a WillModifyNumber method to make sure that restyles happen (see the big doxygen comment requested in comment 157).
Attachment #592573 - Flags: review?(jwatt) → review-
Reporter

Updated

8 years ago
Attachment #592574 - Flags: review?(jwatt) → review+
Reporter

Comment 170

8 years ago
Comment on attachment 592575 [details] [diff] [review]
part 12 - Remove unnecessary serialisation from setting nsSVGInteger

Review of attachment 592575 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGInteger.cpp
@@ +113,5 @@
> +  // attribute value has an associated serialized version (a string value) even
> +  // if the integers match due to the way integers are stored in nsAttrValue.
> +  if (aValue == mBaseVal && mIsBaseSet) {
> +    return;
> +  }

Again, I think we need a WillChangeInteger() call here to make sure restyles happen. r-'ing for now.
Attachment #592575 - Flags: review?(jwatt) → review-
Reporter

Comment 171

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #160)
> The single atom compare on what's already a slow path is not a big deal for
> the non-SVG case.  So I'd just worry about how the SVG code wants stuff to
> work.

Hmm, I hadn't actually noticed that was an atom compare - I'm not sure we really want to turn SVG list types into atoms given the size of the lists some tools output. Anyway, I guess that's a separate issue to pursue outside this bug.
Reporter

Updated

8 years ago
Attachment #592576 - Flags: review?(jwatt) → review+
Reporter

Comment 172

8 years ago
(In reply to Jonathan Watt [:jwatt] from comment #166)
> I think we should just call SetMiscAtomOrString without a check. I *hope*
> all the SVG types check to see if their baseVal is set when asked for their
> value as a string, but if any of them were to forget, we'd end up producing
> non-empty attribute values for default values.

Yeah, so it seems nsSVGAngle doesn't have a check to see if its baseVal is actually set.

Probably it would be good to add a check to your test to make sure that attr="" results in the empty string being returned to script from getAttribute("attr").

> Plus this is really an optimization for attr="" (attribute set to empty
> string), which doesn't seem worth it to me.
Reporter

Comment 173

8 years ago
Comment on attachment 592578 [details] [diff] [review]
part 14 - Remove unnecessary serialisation from setting nsSVGAngle

Review of attachment 592578 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGAngle.cpp
@@ +288,3 @@
>    float valueInUserUnits = mBaseVal * GetDegreesPerUnit(mBaseValUnit);
>    mBaseValUnit = PRUint8(unitType);
> +  SetBaseValue(valueInUserUnits, aSVGElement, false);

Please add a comment noting that you're passing false so that we don't get a double notification.
Attachment #592578 - Flags: review?(jwatt) → review+
Reporter

Updated

8 years ago
Attachment #592581 - Flags: review?(jwatt) → review+
Reporter

Updated

8 years ago
Attachment #592582 - Flags: review?(jwatt) → review+
Reporter

Updated

8 years ago
Attachment #592583 - Flags: review?(jwatt) → review+
Reporter

Updated

8 years ago
Attachment #592584 - Flags: review?(jwatt) → review+
Reporter

Comment 174

8 years ago
Comment on attachment 592585 [details] [diff] [review]
part 21 - Remove unnecessary serialisation from setting SVGPathSegList

Review of attachment 592585 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGPathSeg.cpp
@@ +252,2 @@
>        InternalItem()[1+index] = float(a##propName);                           \
>        NS_ABORT_IF_FALSE(IsInList(), "DidChangePathSegList() is wrong");       \

You can move this abort up to before the WillChangePathSegList() call, and change it to "Will/DidChangePathSegList() is wrong".
Attachment #592585 - Flags: review?(jwatt) → review+
Reporter

Comment 175

8 years ago
Comment on attachment 592586 [details] [diff] [review]
part 22 - Remove unnecessary serialisation from setting SVGTransformList

Review of attachment 592586 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/DOMSVGTransform.cpp
@@ +181,5 @@
>  NS_IMETHODIMP
>  DOMSVGTransform::SetTranslate(float tx, float ty)
>  {
> +  if (mIsAnimValItem)
> +    return NS_ERROR_DOM_NO_MODIFICATION_ALLOWED_ERR;

Not really keen on all this curly bracket removing noise, especially in such a big patch series (and because I prefer them), but I guess you've done it now.

::: content/svg/content/src/DOMSVGTransform.h
@@ +205,5 @@
>    }
>    SVGTransform& Transform() {
>      return HasOwner() ? InternalItem() : *mTransform;
>    }
> +  nsAttrValue NotifyElementWillChange();

If this was inline we'd benefit from better RVO, but not a big deal.
Attachment #592586 - Flags: review?(jwatt) → review+
The landing on inbound caused a 10-15% regression on dromaeo-DOM.  Why?  Nothing in there looked obviously bad to me, but clearly something bad is happening.

Brian, can you please back out of inbound for now, then figure out which of those changes is regressing dromaeo-DOM (and ideally which subtest it's regressing and by how much)?
I backed it out of inbound, for the perf regressions.
Whiteboard: [inbound, parts 1 to 5 only, don't close yet please]
> I'm not sure we really want to turn SVG list types into atoms

We only do that when we have mutation event listeners.  I wouldn't worry about it overmuch for now.

Back to the perf regression.... is it possible that it's due to the cost of having to make a copy of the string every time before doing the "has the value changed?" compare?  If it is (again, testing which patch caused the problem and which tests are affected plus some profiling would likely tell us), we may want to pass both an nsAttrValue and a string to the function that checks for changes, and use whichever one has data.....  Alternately, we could have a way to create an nsAttrValue out of a dependent string somehow, maybe.
Reporter

Comment 180

8 years ago
I took a look at the perf regression to see if I can help speed things along a bit.

It seems to be the dom-attr tests that have regressed: http://dromaeo.com/?dom-attr . Specifically, it seems like 'part 1' regresses the 'setAttribute' and 'element.property = value' tests (they test setting the 'id' attribute to the same value over and over again), and 'part 2' regresses the 'element.property' test (which tests reading the 'id' attribute (same value) over and over again). The subsequent three patches seem to have little effect. You can see the script these tests run at the top of:

view-source:http://dromaeo.com/tests/dom-attr.html

Just from looking at the code in a debugger, there are two things that seem to add overhead in part 1, to me. First in SetAttr there's the line 'nsAttrValue value(aValue)', which causes us to allocate memory for and copy a new string. Then there's the fact that in MaybeCheckSameAttrVal |aValue.EqualsAsStrings(*info.mValue)| is more expensive than the old |info.mValue->Equals(aValue, eCaseMatters)|. The reason that EqualsAsStrings is more expensive is because the nsAttrValue for the 'id' attribute's existing value is of type eAtom, whereas the new nsAttrValue is of type eString. This causes EqualsAsStrings to convert the eAtom value to a string (allocating and copying) for comparison, whereas the old code just used to call Equals() on the atom.

Anyway, I'll get some profiles shortly.
Reporter

Updated

8 years ago
Attachment #595215 - Attachment description: Profile before 'part 1' patch → Profile before 'part 1' patch - formatting screwed up
Attachment #595215 - Attachment is obsolete: true
Reporter

Updated

8 years ago
Attachment #595216 - Attachment description: Profile after 'part 1' patch → Profile after 'part 1' patch - formatting screwed up
Attachment #595216 - Attachment is obsolete: true
Reporter

Updated

8 years ago
Attachment #595255 - Attachment description: Profile after 'part 1' patch → Profile before 'part 1' patch
Reporter

Comment 185

8 years ago
So it seems like before 'part 1', MaybeCheckSameAttrVal completely dominates nsGenericElement::SetAttr, then after 'part 1' the nsAttrValue ctor and dtor calls dominate, *even though MaybeCheckSameAttrVal got a bit more expensive*!
> First in SetAttr there's the line 'nsAttrValue value(aValue)', which causes us to
> allocate memory for and copy a new string.

Right, see comment 179.  Looking at your profile data, that's a _huge_ cost.  Just the constructor and destructor for that nsAttrValue in the attached profile are 1.5x what the total time was for the old code!

> This causes EqualsAsStrings to convert the eAtom value to a string

Hmm.  This is definitely wrong, if it's happening.  It looks like lhs and rhs are backwards in EqualsAsStrings?  We want to serialize the one that's already of string type, if any.  That said, this doesn't seem to be a huge hit.
Isn't nsAttrValue::SetTo(const nsAString& aValue) already attempting to share the source string's buffer?
Yes, but the source string in this case is coming from JS, so it's a dependent string pointing to the JSString data and hence has no buffer to share.

We really need to fix that.  :(
Then seems like passing the string alongside the nsAttrValue to MaybeCheckSameAttrVal is the easiest way to fix this.
Assignee

Comment 190

8 years ago
This patch fixes the perf regression with regards to setAttribute along the lines of comment 189. I also tweaked EqualsAsStrings.

(In reply to Boris Zbarsky (:bz) from comment #186)
> It looks like lhs and
> rhs are backwards in EqualsAsStrings?  We want to serialize the one that's
> already of string type, if any.

They're the right way around. nsAttrValue::Equals(const nsAString&, nsCaseTreatment) will avoid serialising its own value if it's a string or atom type and use a dependent string instead.

That said, EqualsAsStrings could be optimised further. After establishing the lhs and rhs, it could check if the rhs is of string/atom type and then use GetStringValue()/GetAtomValue() and pass those to Equals rather than using ToString. It's not relevant to this particular test anymore now that I've followed Rob's suggestion and passing either the string or nsAttrValue along but might still be worth doing.

Not putting this up for review yet because:
* There is still a significant perf regression in createTextNode. I can't figure out why yet.
* I should add the documentation requested in comment 142.
* I need rebase the other patches on top and check it all still works.
* I need to run it through Try and check the perf regression is fixed on other platforms too (currently tested only on Win)
Assignee

Updated

8 years ago
Attachment #594643 - Attachment is obsolete: true
Attachment #594643 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #592951 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #594644 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #592564 - Flags: checkin+
Assignee

Updated

8 years ago
Attachment #592568 - Flags: checkin+
> They're the right way around.

Ah, with the atom check added, agreed.

> There is still a significant perf regression in createTextNode.

Erm.  That's pretty odd.  From which patch, just part 1?
Assignee

Comment 192

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #191)
> > There is still a significant perf regression in createTextNode.
> 
> Erm.  That's pretty odd.  From which patch, just part 1?

Yes, just part 1. It is odd and I think it's just noise. I've run it a few more times and the results vary alot (the error range dromaeo gives me is about 40% for that result). Looking at it in a profiler, most of the work is in XPC and I can't see any other code that this patch touches.

I'm going to ignore it for now and see what results Try gives me later.
Assignee

Comment 193

8 years ago
(In reply to Brian Birtles (:birtles) from comment #190)
> * I need rebase the other patches on top and check it all still works.

I guess I didn't think through these changes at all. The first few patches in this series are all about replacing nsAString with nsAttrValue.

The problem, of course, is that this is currently expensive for strings from JS (comment 188).

So possible solutions:
a) Allow sharing string buffers with JSString data--this sounds like it would take a long time and bz assures me we shouldn't wait for that to happen.

b) Make a stack class that wraps a pointer to a string or an nsAttrValue and pass that to BeforeSetAttr etc. It could have convenience methods for getting a string out of it etc.

c) Make a stack subclass of nsAttrValue that can be initialised with a dependent string. This is bz's suggestion. I'm not quite clear on all the details but I guess we could do it without making method calls virtual by putting all the functionality into nsAttrValue itself.

After talking with bz we think (c) is worth a try. Jonas, what do you think?
So what I was thinking of is that we have a stack-only subclass of nsAttrValue that can be initialized with a |const nsAString*|.  It would use protected methods on the nsAttrValue to initialize it with that pointer.  We'd ned a new attr value type id for this "dependent string" case.  And then we'd need to change various logic in nsAttrValue to handle this new value type....  I think it should work ok.
Assignee

Comment 195

8 years ago
Removed the passing of both strings and nsAttrValue to MaybeCheckSameAttrVal since it's not going to work for us after all. Added documentation to nsAttrValue.h.
Assignee

Updated

8 years ago
Attachment #595294 - Attachment is obsolete: true
Assignee

Comment 196

8 years ago
(In reply to Jonathan Watt [:jwatt] from comment #157)
> Comment on attachment 592571 [details] [diff] [review]
> part 8 - Remove unnecessary serialisation from setting nsSVGLength2
> Again, a Doxygen style comment would be good:
> 
> /**
>  * Helper methods for the type-specific DidChangeXXX methods.
>  *
>  * aEmptyOrOldValue must be the object returned from the corresponding
>  * WillChangeXXX call. If we have mutation event listeners things will go
>  * wrong if it isn't.
>  */

I expanded this comment a little since "things will go wrong" sounds a little mysterious.
 
> @@ +1322,5 @@
> > +
> > +void
> > +nsSVGElement::DidChangeValue(nsIAtom* aName, const nsAttrValue& aOldValue,
> > +                             nsAttrValue& aNewValue)
> > +{
> 
> It would be good if we could have an NS_ABORT_IF_FALSE below checking that
> if we have mutation listeners, aEmptyOrOldValue is not uninitialized. I'm
> not really sure how you can do that currently though, since the best
> nsAttrValue seems to offer is IsEmptyString(), which is not what we want.
> Maybe we can add a bogus "not initialized" type to nsAttrValue purely for
> this purpose? Boris/Jonas?

It would be nice to be able to test we're doing the right thing here with an assertion. However, I wonder if what we have is enough for now. Firstly, nothing horrible will happen if aOldValue is an empty string since SetAttrAndNotify already handle empty strings. The problem is just correctness and for now the tests I've added should cover this (i.e. we're checking the oldValue member of mutation events for a pretty good range of mutations). Maybe it's sufficient to rely on the tests for now?

> @@ +346,4 @@
> >    float valueInUserUnits = 
> >      mBaseVal / GetUnitScaleFactor(aSVGElement, mSpecifiedUnitType);
> >    mSpecifiedUnitType = PRUint8(unitType);
> > +  SetBaseValue(valueInUserUnits, aSVGElement, false);
> 
> Hmm, under SetBaseValue() we call SetBaseValueInSpecifiedUnits(), so we'll
> get two Will/DidModifyLength calls for each ConvertToSpecifiedUnits() call...

No it's fine since aDoSetAttr is false. We have tests that would catch this if we were sending 
duplicate notifications.
Attachment #592571 - Attachment is obsolete: true
Attachment #595337 - Flags: review?(jwatt)
Assignee

Updated

8 years ago
Blocks: 725227
Reporter

Comment 197

8 years ago
(In reply to Boris Zbarsky (:bz) from comment #186)
> Right, see comment 179.

Sorry, I didn't mean to ignore your observations, I was just trying to provide more info. :)
Component: SVG → Style System (CSS)
Reporter

Comment 198

8 years ago
I did some profiling of the 'part 2' regression this morning. I was seeing a fairly significant drop (50 -> 40) yesterday, but I'm not seeing that today (I'm still using the same patches that were checked in on m-i). Something that I should have been keeping constant must have varied yesterday and given me bad results.

The measurements I got today are as follows (number proceeding each group of three lines indicates the patch applied for that measurement - "0" means no patches):

0:
33.85  44.28  32.35  20.03  12.30  28.06  http://dromaeo.com/?id=162807
33.30  42.88  32.94  20.29  12.18  27.92  http://dromaeo.com/?id=162809
33.33  41.64  31.62  20.08  12.33  27.71  http://dromaeo.com/?id=162810
1:
31.30  42.98  15.99  11.50  12.15  28.28  http://dromaeo.com/?id=162803
31.84  38.61  16.09  11.45  11.76  28.53  http://dromaeo.com/?id=162804
31.88  43.01  16.13  11.38  11.94  28.38  http://dromaeo.com/?id=162805
2:
32.44  46.34  16.11  11.93  12.37  28.90  http://dromaeo.com/?id=162800
31.45  44.22  15.76  11.83  12.56  28.65  http://dromaeo.com/?id=162801
31.87  41.78  15.94  11.91  12.54  29.09  http://dromaeo.com/?id=162802
5:
31.27  43.77  16.21  12.13  12.07  28.02  http://dromaeo.com/?id=162814
31.23  38.84  16.23  12.10  12.24  27.63  http://dromaeo.com/?id=162815
30.79  42.26  16.53  12.06  12.19  28.14  http://dromaeo.com/?id=162816

So it seems like only part 1 regresses the dom-attr tests, and only the 'setAttribute' and 'element.property = value' subtests.
Assignee: birtles → nobody
Component: Style System (CSS) → SVG
Reporter

Comment 199

8 years ago
(What is going on with these random field changes - seriously, I never touched anything at the top of the bug!)
Assignee: nobody → birtles
Assignee

Updated

7 years ago
Attachment #592569 - Attachment is obsolete: true
Attachment #592569 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #595630 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #595631 - Attachment description: part 7 - Remove unnecessary serialisation from setting SVGEnum; → part 7 - Remove unnecessary serialisation from setting SVGEnum; r=jwatt
Assignee

Updated

7 years ago
Attachment #594645 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #595337 - Attachment is obsolete: true
Attachment #595337 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #592572 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592573 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #595632 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #592574 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #595634 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #592575 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592576 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #595636 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #592578 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592579 - Attachment is obsolete: true
Attachment #592579 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #592580 - Attachment is obsolete: true
Attachment #592580 - Flags: review?(jwatt)
Assignee

Updated

7 years ago
Attachment #592581 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592582 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592583 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592584 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592585 - Attachment is obsolete: true
Assignee

Updated

7 years ago
Attachment #592586 - Attachment is obsolete: true