Investigate if nsXULElement::mPrototype could be removed

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments, 9 obsolete attachments)

19.09 KB, patch
Details | Diff | Splinter Review
all
75.17 KB, patch
Details | Diff | Splinter Review
4.08 KB, patch
Details | Diff | Splinter Review
31.64 KB, patch
Details | Diff | Splinter Review
15.49 KB, patch
Details | Diff | Splinter Review
7.83 KB, patch
Details | Diff | Splinter Review
2.54 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Need to check txul and ts at least and also memusage.
(Assignee)

Comment 1

5 years ago
Created attachment 642235 [details] [diff] [review]
part 1, Remove nsIScriptEventHandlerOwner
(Assignee)

Comment 2

5 years ago
Created attachment 642236 [details] [diff] [review]
part 2, make nsNodeUtils do less random stuff
(Assignee)

Comment 3

5 years ago
Created attachment 642237 [details] [diff] [review]
part 3, remove nsXULElement::mPrototype
(Assignee)

Comment 4

5 years ago
Created attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr
(Assignee)

Comment 5

5 years ago
Created attachment 642240 [details] [diff] [review]
part 4, with -w
(Assignee)

Comment 6

5 years ago
Created attachment 642241 [details] [diff] [review]
all
(Assignee)

Comment 7

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

Comment 8

5 years ago
Try builds http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-4e4e943c5bbe/
(Assignee)

Comment 9

5 years ago
Do we have any startup or new-window perf tests anymore?
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.
(Assignee)

Comment 11

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

Comment 12

5 years ago
But perhaps I should improve cloning case too, so that XBL creation stays fast.
(Assignee)

Comment 13

5 years ago
Problem is that I don't know any tests for this. Did we stop running startup and new window tests?
(Assignee)

Comment 14

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

Comment 15

5 years ago
Created attachment 642369 [details] [diff] [review]
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)
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=c592006d591c
(Assignee)

Comment 17

5 years ago
With talos https://tbpl.mozilla.org/?tree=Try&rev=f1aaed308b5b
(Assignee)

Comment 18

5 years ago
Apparently we do have still some kind of startup test, ts_paint, and it isn't affected by the change.
(Assignee)

Comment 19

5 years ago
Created attachment 642374 [details] [diff] [review]
all
Attachment #642241 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
QA Contact: bugs
(Assignee)

Updated

5 years ago
Assignee: nobody → bugs
QA Contact: bugs
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.
(Assignee)

Comment 21

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

Comment 23

5 years ago
> XULElement::Clone currently only calls SetAttr on the attributes not stored
> in mPrototype.
Ah, indeed.
(Assignee)

Comment 24

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

Comment 25

5 years ago
XBL stuff of course do clone. Maybe you're talking about that.
(Assignee)

Comment 26

5 years ago
Created attachment 642428 [details] [diff] [review]
part 2, v2 (needs more testing)
Attachment #642236 - Attachment is obsolete: true
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()
(Assignee)

Comment 28

5 years ago
Created attachment 642441 [details] [diff] [review]
part 6, faster cloning

This is a bit ugly.

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

Comment 29

5 years ago
Created attachment 642442 [details] [diff] [review]
all
Attachment #642374 - Attachment is obsolete: true
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.
(Assignee)

Comment 31

5 years ago
Sure. It would be ugly just once, not twice :)
(Assignee)

Comment 32

5 years ago
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)
(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.
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??
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
(Assignee)

Comment 36

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

Comment 37

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

Comment 38

5 years ago
about:memory right after startup doesn't show any difference.
(Assignee)

Comment 39

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

Comment 40

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

Comment 42

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

Updated

5 years ago
Attachment #642235 - Flags: review?(bzbarsky)
(Assignee)

Comment 43

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

Comment 44

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

Comment 45

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

Comment 46

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

Comment 47

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

This is very similar to what happens in ::Create and ::MakeHeavyWeight.
Attachment #642441 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Blocks: 774724
Comment on attachment 642235 [details] [diff] [review]
part 1, Remove nsIScriptEventHandlerOwner

r=this-is-so-awesome
Attachment #642235 - Flags: review?(bzbarsky) → review+
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.
Attachment #642428 - Flags: review?(bzbarsky) → review+
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?
Attachment #642237 - Flags: review?(bzbarsky) → review+
Oh, one other comment.  Please remember to drop the "try" bits from the checkin comments and add the bug number?
Comment on attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr

Where did the RemoveFromIdTable() from UnsetAttr migrate to?
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.
Attachment #642369 - Flags: review?(bzbarsky) → review+
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.  :(
Attachment #642441 - Flags: review?(bzbarsky) → review+
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!
Attachment #642238 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 56

5 years ago
Created attachment 643767 [details] [diff] [review]
part 2
Attachment #642428 - Attachment is obsolete: true
(Assignee)

Comment 57

5 years ago
Created attachment 643768 [details] [diff] [review]
part 3
Attachment #642237 - Attachment is obsolete: true
(Assignee)

Comment 58

5 years ago
Created attachment 643770 [details] [diff] [review]
part 4
Attachment #642238 - Attachment is obsolete: true
Attachment #642240 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #643767 - Attachment description: v2 → part 2
(Assignee)

Comment 59

5 years ago
Created attachment 643775 [details] [diff] [review]
part 5
Attachment #642369 - Attachment is obsolete: true
(Assignee)

Comment 60

5 years ago
Created attachment 643776 [details] [diff] [review]
part 6
Attachment #642441 - Attachment is obsolete: true
(Assignee)

Comment 61

5 years ago
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
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 671192

Updated

5 years ago
Blocks: 672258
(Assignee)

Updated

5 years ago
Depends on: 775972
You need to log in before you can comment on or make changes to this bug.