Last Comment Bug 773945 - Investigate if nsXULElement::mPrototype could be removed
: Investigate if nsXULElement::mPrototype could be removed
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
:
:
Mentors:
Depends on: 775972
Blocks: 774724 671192 672258
  Show dependency treegraph
 
Reported: 2012-07-14 04:27 PDT by Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
Modified: 2012-07-20 14:56 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1, Remove nsIScriptEventHandlerOwner (19.09 KB, patch)
2012-07-14 09:16 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
part 2, make nsNodeUtils do less random stuff (2.14 KB, patch)
2012-07-14 09:16 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 3, remove nsXULElement::mPrototype (31.66 KB, patch)
2012-07-14 09:17 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
part 4, remove nsXULElement::UnsetAttr (15.86 KB, patch)
2012-07-14 09:18 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
part 4, with -w (12.51 KB, patch)
2012-07-14 09:20 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
all (64.35 KB, patch)
2012-07-14 09:21 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 5, cleanup prototype nodes (7.75 KB, patch)
2012-07-15 05:07 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
all (71.43 KB, patch)
2012-07-15 06:12 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 2, v2 (needs more testing) (3.92 KB, patch)
2012-07-15 14:26 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
part 6, faster cloning (2.56 KB, patch)
2012-07-15 15:49 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
bzbarsky: review+
Details | Diff | Splinter Review
all (75.17 KB, patch)
2012-07-15 15:51 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 2 (4.08 KB, patch)
2012-07-19 02:05 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 3 (31.64 KB, patch)
2012-07-19 02:11 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 4 (15.49 KB, patch)
2012-07-19 02:15 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 5 (7.83 KB, patch)
2012-07-19 02:19 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review
part 6 (2.54 KB, patch)
2012-07-19 02:22 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details | Diff | Splinter Review

Description Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 04:27:16 PDT
Need to check txul and ts at least and also memusage.
Comment 1 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 09:16:04 PDT
Created attachment 642235 [details] [diff] [review]
part 1, Remove nsIScriptEventHandlerOwner
Comment 2 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 09:16:45 PDT
Created attachment 642236 [details] [diff] [review]
part 2, make nsNodeUtils do less random stuff
Comment 3 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 09:17:24 PDT
Created attachment 642237 [details] [diff] [review]
part 3, remove nsXULElement::mPrototype
Comment 4 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 09:18:04 PDT
Created attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr
Comment 5 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 09:20:04 PDT
Created attachment 642240 [details] [diff] [review]
part 4, with -w
Comment 6 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 09:21:36 PDT
Created attachment 642241 [details] [diff] [review]
all
Comment 7 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 10:31:40 PDT
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] (way behind * queues, especially ni? queue) 2012-07-14 14:21:02 PDT
Try builds http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/opettay@mozilla.com-4e4e943c5bbe/
Comment 9 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-14 18:15:59 PDT
Do we have any startup or new-window perf tests anymore?
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-15 01:48:08 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 03:11:47 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 03:17:27 PDT
But perhaps I should improve cloning case too, so that XBL creation stays fast.
Comment 13 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 03:18:10 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 03:43:57 PDT
(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] (way behind * queues, especially ni? queue) 2012-07-15 05:07:26 PDT
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)
Comment 16 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 05:48:13 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c592006d591c
Comment 17 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 06:01:10 PDT
With talos https://tbpl.mozilla.org/?tree=Try&rev=f1aaed308b5b
Comment 18 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 06:05:39 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 06:12:25 PDT
Created attachment 642374 [details] [diff] [review]
all
Comment 20 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-15 13:10:51 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 13:35:32 PDT
(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 2012-07-15 13:52:41 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 14:02:56 PDT
> XULElement::Clone currently only calls SetAttr on the attributes not stored
> in mPrototype.
Ah, indeed.
Comment 24 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 14:05:30 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 14:12:06 PDT
XBL stuff of course do clone. Maybe you're talking about that.
Comment 26 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 14:26:37 PDT
Created attachment 642428 [details] [diff] [review]
part 2, v2 (needs more testing)
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-15 14:30:34 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 15:49:55 PDT
Created attachment 642441 [details] [diff] [review]
part 6, faster cloning

This is a bit ugly.

https://tbpl.mozilla.org/?tree=Try&rev=07ed4d6c78cb
Comment 29 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 15:51:57 PDT
Created attachment 642442 [details] [diff] [review]
all
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-07-15 16:18:17 PDT
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] (way behind * queues, especially ni? queue) 2012-07-15 16:25:55 PDT
Sure. It would be ugly just once, not twice :)
Comment 32 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-15 16:33:16 PDT
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: gavin@gavinsharp.com] 2012-07-15 17:24:12 PDT
(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 2012-07-15 18:09:10 PDT
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 2012-07-16 02:08:19 PDT
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] (way behind * queues, especially ni? queue) 2012-07-16 07:06:50 PDT
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] (way behind * queues, especially ni? queue) 2012-07-16 07:09:22 PDT
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] (way behind * queues, especially ni? queue) 2012-07-16 10:09:18 PDT
about:memory right after startup doesn't show any difference.
Comment 39 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-16 10:15:53 PDT
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] (way behind * queues, especially ni? queue) 2012-07-16 15:52:28 PDT
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 2012-07-17 01:46:22 PDT
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] (way behind * queues, especially ni? queue) 2012-07-17 07:30:39 PDT
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] (way behind * queues, especially ni? queue) 2012-07-17 09:46:11 PDT
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] (way behind * queues, especially ni? queue) 2012-07-17 09:49:10 PDT
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] (way behind * queues, especially ni? queue) 2012-07-17 09:50:20 PDT
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] (way behind * queues, especially ni? queue) 2012-07-17 09:52:20 PDT
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] (way behind * queues, especially ni? queue) 2012-07-17 09:53:38 PDT
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 [:bz] (still a bit busy) 2012-07-17 11:01:31 PDT
Comment on attachment 642235 [details] [diff] [review]
part 1, Remove nsIScriptEventHandlerOwner

r=this-is-so-awesome
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 11:03:26 PDT
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 [:bz] (still a bit busy) 2012-07-17 11:10:05 PDT
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 [:bz] (still a bit busy) 2012-07-17 11:10:56 PDT
Oh, one other comment.  Please remember to drop the "try" bits from the checkin comments and add the bug number?
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 11:31:03 PDT
Comment on attachment 642238 [details] [diff] [review]
part 4, remove nsXULElement::UnsetAttr

Where did the RemoveFromIdTable() from UnsetAttr migrate to?
Comment 53 Boris Zbarsky [:bz] (still a bit busy) 2012-07-17 11:45:40 PDT
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 [:bz] (still a bit busy) 2012-07-17 11:47:53 PDT
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 [:bz] (still a bit busy) 2012-07-17 23:10:00 PDT
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] (way behind * queues, especially ni? queue) 2012-07-19 02:05:53 PDT
Created attachment 643767 [details] [diff] [review]
part 2
Comment 57 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-19 02:11:02 PDT
Created attachment 643768 [details] [diff] [review]
part 3
Comment 58 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-19 02:15:01 PDT
Created attachment 643770 [details] [diff] [review]
part 4
Comment 59 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-19 02:19:12 PDT
Created attachment 643775 [details] [diff] [review]
part 5
Comment 60 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2012-07-19 02:22:15 PDT
Created attachment 643776 [details] [diff] [review]
part 6

Note You need to log in before you can comment on or make changes to this bug.