Closed Bug 20724 Opened 22 years ago Closed 22 years ago
[perf] optimize XUL attribute construction
Atomize short values; pool nsXULAttribute objects; etc.
Severity: normal → major
Status: NEW → ASSIGNED
Priority: P3 → P1
Summary: [perf] optimize XUL attribute construction → [perf] [DOGFOOD] optimize XUL attribute construction
Target Milestone: M12
we want this, but not dogfood...
bizarre. the bug depending on it is PDT+.
This is PDT+. This bug was filed to specifically address 9489 (which is PDT+).
The attached patch atomizes short attribute values. It uses a tagged pointer: the least-significant bit indicates whether the "mValue" points to a raw Unicode string or an nsIAtom. On the bookmarks window, with a 12 character cutoff, we get a 10-to-1 ratio of atoms to strings while scrolling down four times. Furthermore the atom hit ratio is incredible: in the 2,000 atom references, and only two unique atoms are created. This data indicates that "12" seems to be a good magic number: we good space (and hence time) savings from atomization without wasting time atomizing rare strings. This yields a net speedup of about 2%.
Looks good. One optimization I could suggest is that you sanity check in SetValueInternal if the values are the same and don't release/addref, since you might delete the atom and remalloc one. This is just a nitpick though. Consider it reviewed.
Also, I'd love to see a new tree widget profile. With both the recycler and this put into the tree, I want to see what we're taking the hit on now.
As to the addref/release atom thing: I purposefully addref the atom *before* assigning to the new value so that the atom (if already existing) will not be decached from the atom table. The degenerate "set-same-value" case results in a redundant store and some addref/releases, but those are really just noise. My feeling was that a check-for-equality adds expense to the common case. Make the degenerate case pay! As to the profiles: got 'em. Come by whenever and we'll look.
Summary: [perf] [DOGFOOD] optimize XUL attribute construction → [perf] optimize XUL attribute construction
Whiteboard: [PDT+] 12/09 → 12/09
Target Milestone: M12 → M13
Value atomization optimization checked in, a=chofmann. Removing [DOGFOOD] and [PDT+] grafitti, because further attribute optimization will yield overall gains of only 2-3% according to latest profiles. (Still worth doing, but there are bigger fish to fry first, and the dependant bug has been marked "fixed", anyway.)
hyatt: gimme some code review lovin' for the fixed size allocator patch. it completely takes nsXULAttribute construction time out of the picture (boosts scrolling by 2-3%). Yay!
Looks good stud.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
fixed size allocator for nsXULAttribute objects checked in.
BULK MOVE: Changing component from XUL to XP Toolkit/Widgets: XUL. XUL component will be deleted.
Component: XUL → XP Toolkit/Widgets: XUL
please ignore, massive spam giving firstname.lastname@example.org backlog of XPToolkits resolved fixed bugs to verify
QA Contact: paulmac → jrgm
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: jrgmorrison → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.