26429 - put XUL elements on a diet

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Chris Waterson]

XUL elements currently require 92 bytes per element. That's

40 bytes for 10 32-bit fields
4 bytes for the mRefCnt
38 bytes for vtable overhead.

Explore:

1. packing some of the fields.
2. removing some of the fields (e.g., mDocument)
3. delegating rarely-used interfaces to an aggregate pointer that is kept in 
the mSlots member.

Comment 1

[Scott Collins]

Make that 48 bytes for vtable overhead.  Waterson knew that... it was just a 
typo.

Comment 2

[Paul MacQuiddy]

spam, open xptoolkit qa contact moving over to jrgm

Comment 3

[Marko Macek]

QueryInterface in nsXULElement seems to occur quite high in performance 
measurements (with jprof on Linux).

Comment 4

[Chris Waterson]

Marko: who are the big callers?

Comment 5

[Marko Macek]

common caller seems to be SelectorMatches 
(I need to to a jprof again and will post it, it may have been improved
recently).

Two more observations:
- Changing nsID::Equals to compare pointers first seems to be a slight
improvement.

- GetIID functions seem to follow the pattern:

static const nsIID& GetIID() { static nsIID iid = NS_IEDITOR_IID; return iid;}

egcs-1.1.2 is unable to inline this (I haven't managed).
If I change the iid to be object static variable, the inlining is possible.
Does doing this make sense?

I haven't tried a newer gcc yet.

Comment 6

[Chris Waterson]

Bug 71530 removes two fields from the XUL element's slots.

Comment 7

[Chris Waterson]

mPrototype and mSlots are mutually exclusive, and should be a |union|.

Comment 8

[jag (Peter Annema)]

Is the union of mSlots and mPrototype still a valid suggestion?

Comment 9

[Jan Varga [:janv]]

Attachment: patch

I noticed that mLineNo is always -1 for nsXULPrototypeElement
so I moved it from nsXULPrototypeNode to nsXULPrototypeScript
I haven't crashed yet with this patch :)

It should save some memory.

What do you think ?

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 100280 [details] [diff] [review]
patch

Where did the FastLoad support for script line numbers go?

The XUL prototype document fastload version identifier needs to change for any
incompatible change to the FastLoad file format (such as removing 32 bits of
line number data from each element's serialization, including it only for
script elements).

/be

Comment 11

[Jan Varga [:janv]]

brendan, did you mean something like this ?

-#define MFL_FILE_VERSION        3       // fix to store dependency mtimes 
+#define MFL_FILE_VERSION        4       // removed line numbers from elements

and there is still support for script line numbers, I didn't remove that.

Comment 12

[Brendan Eich [:brendan]]

No, I meant change XUL_FASTLOAD_FILE_VERSION by -1 -- see
http://lxr.mozilla.org/mozilla/source/content/xul/document/public/nsIXULPrototypeCache.h#136
(the macro you changed versions the XUL-independent, XPCOM-generic
superstructure format of the FastLoad file, but that format didn't change in
your patch).

Good call on fixing this, btw -- I missed the independent mLineNo serialization
and deserialization for script nodes that you left intact (I thought scripts
partook of the Read32 and Write32 calls that you removed).

I'm attaching my version of your patch, which fixes a mislocated memset that
could crash on out-of-memory, in nsXULElement.cpp.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: vargas' patch plus XUL_FASTLOAD_FILE_VERSION fix + memset fix

Hoping Ben will review as module owner in this case.

/be

Comment 14

[Jan Varga [:janv]]

Attachment: all in one + PRBool -> PRPackedBool

Thanks for help.
I converted PRBools to PRPackedBools too.

Comment 15

[Brendan Eich [:brendan]]

You might even change mLineNo to a PRUint16 and pack it next to the
PRPackedBools for best space efficiency.  I doubt we support JS files or XUL
files containing scripts that have > 64K lines; the JS engine does not.

/be

Comment 16

[Jan Varga [:janv]]

Attachment: new patch

new patch incorporating brendan's last suggestion

Comment 17

[Jan Varga [:janv]]

could I get r/sr on this ?

Comment 18

[Jan Varga [:janv]]

err, new patch coming

Comment 19

[Jan Varga [:janv]]

Attachment: fix

sorry for SPAM

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 100387 [details] [diff] [review]
fix

sr=[e-mail], this looks good to me.	Ben is busy with university
work for a few days, but should be back to r= this soon.  If you want it
sooner, maybe hyatt will do the deed.

/be

Comment 21

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 100387 [details] [diff] [review]
fix

r=[e-mail]

Comment 22

[Jan Varga [:janv]]

Comment on attachment 100387 [details] [diff] [review]
fix

patch has been checked in

Comment 23

[Jerome Lacoste]

could this bug be the reason for the big perf regression in bug 170704

Comment 24

[Jan Varga [:janv]]

Attachment: additional patch

attaching patch that I had to check in to fix startup regression

Comment 25

[Chris Waterson]

re: comment #8, I'm not sure if it's valid anymore.  When I first wrote this,
lightweight XUL elements would have a prototype pointer (and no slots), and
heavyweight XUL elements would have a slots pointer (and no prototype).  Since
then, shaver and I changed stuff so that altering an attribute's value wouldn't
necessarily cause the XUL element to "fault" and become heavy.  So now, it may
be possible for an element to have both a prototype pointer and a slots
pointer...but it's been so long since I've looked at this that I can't really
remember.

Comment 26

[Jan Varga [:janv]]

> So now, it may be possible for an element to have both a prototype pointer and
> a slots pointer...

from what I saw in the code I think this is the case.

Comment 27

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Yes, that's the case.

Comment 28

[Philip K. Warren]

There was a misspelling that accidentally went in with the checkin for this patch:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsXULElement.cpp&root=/cvsroot&subdir=mozilla/content/xul/content/src&command=DIFF_FRAMESET&rev1=1.428&rev2=1.429

nsXULProtlotypeAttribute::gNumElements should be
nsXULPrototypeAttribute::gNumElements. I don't know if this affects any builds
since that code is only used if XUL_PROTOTYPE_ATTRIBUTE_METERING is defined.

Comment 29

[Jan Varga [:janv]]

hmm, I'll fix it.
thanks for info

Comment 30

[Jan Varga [:janv]]

Attachment: patch to fixo a typo

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 102334 [details] [diff] [review]
patch to fixo a typo

Honor system says go ahead and check this in once the tree opens to approved
1.2beta checkins.

/be

Comment 32

[nhottanscp]

please get an approval from [e-mail] before checking in

Comment 33

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Brendan already gave such an approval, right above your comment.  Jan's good to
go with this fix, as soon as we open for approved checkins.

Comment 34

[nhottanscp]

then please put 'a=' (also 'r=' and 'sr=') in check in comment to avoid
confusion, thanks.

Comment 35

[Jan Varga [:janv]]

sorry about that, I requested an approval along with other bugs.
do I really need r/sr for such a typo ?

Comment 36

[Brendan Eich [:brendan]]

varga: no, you don't need r= and sr= for typo fixes like this (honor system was
the phrase I used to thumbnail why you don't), but nhotta was sheriff and wanted
to see r=/sr=/a=[e-mail] in your CVS checkin comment.  As a nicety so
the sheriff doesn't panic, this seems worth doing to me.

/be

Comment 37

[Jan Varga [:janv]]

ok, thanks for the explanation.
I will keep it in my mind in future.

Comment 38

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

Just FYI, nowadays XUL Element takes 44 bytes (32bit system), and
bug 363067 will reduce that still 4 bytes.

On 1.8 branch the size is 52 bytes.

Comment 39

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

Bug 363067 is now fixed. Making nsXULElement even smaller is hard.
Marking this fixed.