773945 - Investigate if nsXULElement::mPrototype could be removed

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Need to check txul and ts at least and also memusage.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 1, Remove nsIScriptEventHandlerOwner

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 2, make nsNodeUtils do less random stuff

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 3, remove nsXULElement::mPrototype

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 4, remove nsXULElement::UnsetAttr

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 4, with -w

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: all

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

John, this is the bug which might need a awsy run. Though, awsy tests probably more
memusage related to page load, not browser chrome.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Try builds http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-4e4e943c5bbe/

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Do we have any startup or new-window perf tests anymore?

Comment 10

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

You could keep a bunch of the perf-savings if you added some code to XULElement::Clone which copied the attribute-list manually and copied over the appropriate flags, rather than calling CopyInnerTo which calls SetAttr for each attribute.

I.e. you'd have to create similar code to that of nsXULElement::Create which sets various flags based on if attributes exist. But I think it could make cloning significantly faster, which I believe is one of the main benefits of prototype attributes.


I'm also a bit worried about losing the nsIScriptEventHandlerOwner. But I never fully understood that so I admittedly don't have a good idea of how much it matters.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Cloning hasn't been the main target for prototypes, but creating elements from prototype cache and
keeping memusage lower. Even with the patch most of the data which attributes hold is still shared,
since xul attributes tend to be strings and atoms which are refcounted.
Prototype cache isn't going anywhere, it is just that xul elements would become heavyweight
immediately.

nsIScriptEventHandlerOwner is about sharing compiled onfoo handlers between windows.
If we don't have prototype, we can't have that either, at least not this way.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

But perhaps I should improve cloning case too, so that XBL creation stays fast.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Problem is that I don't know any tests for this. Did we stop running startup and new window tests?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Olli Pettay [:smaug] from comment #12)
> But perhaps I should improve cloning case too, so that XBL creation stays
> fast.
Ah, but even now we end up copying all the attributes using SetAttr.
That is what keeps various flags correct.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 5, cleanup prototype nodes

On 32 bit this drops PrototypeElement from 36 to 32
(and at least on jemalloc2 we could use 32 bucket, not 48 anymore)
PrototypeAttribute drops from 12 to 8 (jemalloc bucket from 16 to 8)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=c592006d591c

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

With talos https://tbpl.mozilla.org/?tree=Try&rev=f1aaed308b5b

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Apparently we do have still some kind of startup test, ts_paint, and it isn't affected by the change.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Attachment: all

Comment 20

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

I think the prototype was created both to improve speed of wrapping a prototype document, as well as improve speed of cloning. At least it used to be the case that the front-end code would clone large DOM trees when opening new windows etc, but maybe that isn't the case any more. But the XBL code would benefit from fast cloning as you mention.

Given how relatively little code is needed, I suspect it's worth avoiding going through SetAttr for both XULElement::Create(prototype) and XULElement::Clone. But I could be wrong.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #20)
> I think the prototype was created both to improve speed of wrapping a
> prototype document, as well as improve speed of cloning. At least it used to
> be the case that the front-end code would clone large DOM trees when opening
> new windows etc,
No. Front-end code uses same xul documents, so those must be loaded quickly.
That is why I'm not removing XUL prototype cache.

> 
> Given how relatively little code is needed, I suspect it's worth avoiding
> going through SetAttr for both XULElement::Create(prototype) and
> XULElement::Clone. But I could be wrong.
XULElement::Clone does already use SetAttr for all the attributes. I'm not changing that.

Comment 22

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

Ok, if you are sure that the front-end code never clones large DOM trees then you know more than me, which certainly is possible :) I know how the prototype cache works, but I thought we also cloned DOM trees in some cases.

XULElement::Clone currently only calls SetAttr on the attributes not stored in mPrototype. As far as I can tell, the current patches makes us call SetAttr on all attributes. Note that CopyInnerTo only uses mAttrsAndChildren.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

> XULElement::Clone currently only calls SetAttr on the attributes not stored
> in mPrototype.
Ah, indeed.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Btw, this is the first time I heard that UI would be cloning large DOM trees.
Also, I haven't seen cloning mentioned in the bugs where prototype stuff was implemented.

I would be more worried about the ::Create case. And that is why I'm testing startup and
hopefully, if I found any tests, also window opening.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

XBL stuff of course do clone. Maybe you're talking about that.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 2, v2 (needs more testing)

Comment 27

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

No, I'm talking about that I thought that we created large chunks of UI by cloning existing UI pieces. I.e. literally having front-end JS code call someXULNode.clone()

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 6, faster cloning

This is a bit ugly.

https://tbpl.mozilla.org/?tree=Try&rev=07ed4d6c78cb

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Attachment: all

Comment 30

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

One way to reduce the uglyness might be to make prototype elements store their attributes using an nsAttrAndChildArray rather than using nsXULPrototypeAttribute. That way we could use the same code-path copy attributes from the prototype element, as from the cloned element.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Sure. It would be ugly just once, not twice :)

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

But for performance testing purposes the patch should be ok.

So far I haven't seen ts_paint regressions, but it is not clear to me what test that is.
We used to have ts and txul/twin, but no idea what has happened to those.

awsy might reveal if memory usage increases during startup.

Attribute modifications in XUL should get faster. (And code a lot simpler)

Comment 33

[:Gavin Sharp [email: [e-mail]]]

(In reply to Jonas Sicking (:sicking) from comment #27)
> No, I'm talking about that I thought that we created large chunks of UI by
> cloning existing UI pieces. I.e. literally having front-end JS code call
> someXULNode.clone()

I don't know of any front-end code that does this, FWIW.

Comment 34

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

Honestly, I'm not worried about memory usage at all. A good way to test might be to start up a browser and count the total number of XUL-attributes alive. I bet it's in the order of a few thousand, so even if we had *no* attributes before (i.e. if all attributes just came out of mPrototype), we'd still only use 100K more memory at the most.

Also, if it wasn't clear. I think these patches are awesome! If the prototype element were to hold an nsAttrAndChildArray this wouldn't even be adding any additional uglyness.

Only perf concern I have remaining is the scriptholder thing, and the only reason I worry is because I don't know anything about it. One option for how to keep that infrastructure might be to still keep the mPrototype pointer in nsXULElement, but only use it when accessing compiled scripts. But I have no idea if it's important or not.

Actually, looking at the code, I only see where we write data to mPrototype->mAttrs[x]->mEventHandler. I can't see us using that data anywhere??

Comment 35

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

Oops, my bad, it's used in the function right above:
http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULElement.cpp#678

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

I agree the prototype element should hold a nsAttrAndChildArray. But that change could be done
in a followup. It requires quite a bit changes.

The script holder thing shouldn't be that bad. onfoo handlers are anyway compiled lazily.

mEventHandler is accessed in nsScriptEventHandlerOwnerTearoff::GetCompiledEventHandler.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Btw, the patches don't enable all the goodness I've been thinking.
I hope we can make Get/Set/UnsetAttr non-virtual.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

about:memory right after startup doesn't show any difference.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Loading 10 windows and checking about:memory doesn't show anything bad either.
In some cases no-proto build takes more memory than proto, in some cases it takes less.
All within noise, I'd say.

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

I wrote a simple test which opens 100 windows and checks how long it takes from
window.open to load event. Again, results are noisy, but similarly noisy with or without the patch.

Comment 41

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

Given how hard it is to measure performance, any chance that you could simply count how many times we end up compiling XUL event handlers with and without this patch?

No worries if that's too much of a pain. I'd imagine that this patch is something that we want anyway given how important web page performance is compared to spinning up new XUL UI.

Comment 42

[Olli Pettay [:smaug][[e-mail]]]

When starting browser we compile 2 (!) onfoo handlers using nsScriptEventHandlerOwnerTearoff::CompileEventHandler.
Both are oncommandupdate.

If nothing is done with the original window and a new window is opened we manage to reuse 1 onfoo handler, again oncommandupdate.

When doing something with the UI we do compile more onfoos lazily, and then some of them can be reused in the new windows.

This all hints that nsIScriptEventHandlerOwner really isn't too effective nowadays. The code is ancient, from year 1999, bug 13218. That bug was filed when we didn't do lazy onfoo compilation.

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642428 [details] [diff] [review]
part 2, v2 (needs more testing)

Keep the behavior for XUL elements cloned from XBL.
I could file a followup to change mLoadedAsInteractiveData to
mLoadedAsXBL or some such.

Comment 44

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642237 [details] [diff] [review]
part 3, remove nsXULElement::mPrototype

Make stuff XUL elements heavy weight from the beginning.
I kept method name MakeHeavyweight, but it will be changed in a followup bug
which makes nsXULPrototypeElement to have nsAttrsAndChildren array so that
we can use the same method for creating and for cloning xul elements.

Comment 45

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr

Move stuff from ::UnsetAttr to AfterSetAttr's if (!aValue) case.
There is -w version of this patch.

Comment 46

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642369 [details] [diff] [review]
part 5, cleanup prototype nodes

Shrink the size of prototype nodes.
In a followup I'll remove nsXULPrototypeAttribute and make
nsXULPrototypeElement to have AttrsAndChildren

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 642441 [details] [diff] [review]
part 6, faster cloning

This is very similar to what happens in ::Create and ::MakeHeavyWeight.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642235 [details] [diff] [review]
part 1, Remove nsIScriptEventHandlerOwner

r=this-is-so-awesome

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642428 [details] [diff] [review]
part 2, v2 (needs more testing)

Probably better to keep more of the original comment on the member.  r=me with that.

Comment 50

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642237 [details] [diff] [review]
part 3, remove nsXULElement::mPrototype

>+++ b/content/xul/content/src/nsXULElement.cpp
>@@ -204,27 +204,27 @@ already_AddRefed<nsXULElement>
>+    *aResult = element.forget().get();

  element.forget(*aResult);

perhaps?  Same elsewhere.

r=me.  This is good stuff.  I assume some devirtualization is still to come?

Comment 51

[Boris Zbarsky [:bzbarsky]]

Oh, one other comment.  Please remember to drop the "try" bits from the checkin comments and add the bug number?

Comment 52

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr

Where did the RemoveFromIdTable() from UnsetAttr migrate to?

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642369 [details] [diff] [review]
part 5, cleanup prototype nodes

r=me, but note that msvc won't pack a PRUint32 bitfield with a bool bitfield.  So if you want those to pack on Windows, you need to make the boolean things also PRUint32.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642441 [details] [diff] [review]
part 6, faster cloning

r=me, though I'm a bit worried about what happens if we add more flags and this stuff gets out of sync.  :(

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr

>+        // XXX Attribute removal case handles more attributes than changing do.
>+        // See bug 233642.

This comment is wrong.  Just remove it.

We should consider a followup to collapse the cases where the aValue and !aValue branches do the same thing.  But I'm happy for that to be a followup!

r=me.  This is lovely!

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 2

Comment 57

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 3

Comment 58

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 4

Comment 59

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 5

Comment 60

[Olli Pettay [:smaug][[e-mail]]]

Attachment: part 6

Comment 61

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/4d5060c3cc62
https://hg.mozilla.org/mozilla-central/rev/4f0b23405d5d
https://hg.mozilla.org/mozilla-central/rev/e2d995afa7f9
https://hg.mozilla.org/mozilla-central/rev/712450475ae4
https://hg.mozilla.org/mozilla-central/rev/195067e055bb
https://hg.mozilla.org/mozilla-central/rev/6d5150ee06ce