104905 - increase nsXULAttributeValue::kMaxAtomValueLength or force atom more in nsXULAttributeValue::SetValue()

Status: RESOLVED WONTFIX

(Core :: XUL, defect)

Description

[(not reading, please use [e-mail] instead)]

nsXULAttributeValue::kMaxAtomValueLength or force atom more in 
nsXULAttributeValue::SetValue()

I'm looking at the quantify data for opening a msg compose window, and I'm
trying to reduce the number of calls to malloc.

nsXULAttributeValue::SetValue() calls, among other things:
                percent calls   propogate time
ToNewUnicode    67.28   1390    8186.03
NS_NewAtom      13.50   2324    1642.59

looking at SetValue(), if the string is <= kMaxAtomValueLength, we use an atom
as long as forceAtom (the second arg to SetValue) is not true.

I'll attach the list of strings that are failing this test, and how often they
come through.  I think we either need to bump up this constant, or use forceAtom.

from -compose:

     57 XSV scrollbarbutton-icon
     57 XSV scrollbarbutton-box
     38 XSV cmd_renderedHTMLEnabler
     30 XSV rdf:http://home.netscape.com/NC-rdf#Name
     29 XSV popup-internal-box
     26 XSV rdf:charset-menu
     26 XSV button-internal-box
     25 XSV menu-iconic-text
     25 XSV menu-iconic-left
     25 XSV menu-iconic-accel
     24 XSV button-toolbar
     24 XSV button-text-container
     22 XSV button-toolbar-2 top
     18 XSV menuitem-iconic
     18 XSV http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul
     15 XSV scrollbar-up-top
     15 XSV scrollbar-up-bottom
     15 XSV scrollbar-thumb
     15 XSV scrollbar-down-top
     15 XSV scrollbar-down-bottom

I'll attach the whole list, and a list from doing about:blank

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: results from -compose

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: results from -P default about:blank

Comment 3

[Chris Waterson]

Could you hold off on this until we get bug 68871 implemented? (Day or two,
knock on wood!) That may dramatically change the landscape here.

Comment 4

[(not reading, please use [e-mail] instead)]

yes, I'll hold off until #68871 is fixed.

Comment 5

[(not reading, please use [e-mail] instead)]

talking to alecf, he suggests a different approach (following what mork does).

instead of creating atoms if length <= kMaxAtomValueLength, we should create
atoms if length > kMinLength.

the idea being if you atomize "foo", it might be more expensive to atomize a
small string than to leave it a string.

we still want to force atomize certain things, like "id" and "name".

we should investigate to see how nsXULAttributeValues are compared and who calls
nsXULAttributeValue::GetValue() and nsXULAttributeValue::GetValueAsAtom()

Comment 6

[Alec Flett]

well, and even "id" you might not want to atomize - I mean if it takes 4 bytes
to store a pointer to an nsIAtom, and "id" (in unicode) is only 4 bytes, then
why not just store the original string in the space provided by the atom.
I'm imagining
  PRPackedBool isAtom;
union {
  nsIAtom* atom;
  PRUnichar[2] unicharValue;
}
or even store the attributes in ASCII:
PRPackedBool isAtom;
PRPackedBool isAscii;
union {
  nsIAtom* atom;
  PRUnichar[2] unicharValue;
  char[4] charValue;
}

Another optimization I thought of was to have a very fast string table just for
attribute names, and you just keep a pointer to 

Comment 7

[Alec Flett]

oops, got caught up with the first thought without finishing the 2nd.
Essentially I'm suggesting a private atom table, except instead of storing an
nsIAtom*, we store a char* or PRUnichar* that points to the actual string in the
string table. Then you could do raw compares of pointers instead of strcmp(),
but the strings themselves would be easily usable as raw char*/PRUnichar*
pointers. You'd have to do manual refcounts, etc, but I could still see this
working... this of course is only useful if strcmp() between attrbute names is
expensive.

Comment 8

[Chris Waterson]

I had kandrot add this because we were actually seeing many duplicate _short_
strings; at one point about a year ago this was saving about 370KB of bloat in
the browser window alone (measured using trace-malloc, not the bogo-bloat
numbers). We played around with the string size (albeit on the main browser
window only), and 12 seemed to catch most of the values. If this dynamic has
changed, sure let's fix it.

The reason we atomize the |id| value _all_ the time is because the style system
asks for the |id| as an atom (via nsIContent::GetID), so we might as well store
it that way.

Comment 9

[Firefox Bug Husbandry Bot]

Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336

Comment 10

[Neil Deakin]

no longer relevant