Closed Bug 20724 Opened 20 years ago Closed 20 years ago

[perf] optimize XUL attribute construction

Categories

(Core :: XUL, defect, P3, major)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: waterson, Assigned: waterson)

References

Details

(Whiteboard: 12/09)

Attachments

(2 files)

Atomize short values; pool nsXULAttribute objects; etc.
Blocks: 9489
Severity: normal → major
Status: NEW → ASSIGNED
Priority: P3 → P1
Summary: [perf] optimize XUL attribute construction → [perf] [DOGFOOD] optimize XUL attribute construction
Target Milestone: M12
Whiteboard: PDT-
we want this, but not dogfood...
bizarre. the bug depending on it is PDT+.
Whiteboard: PDT- → PDT+
This is PDT+.  This bug was filed to specifically address 9489 (which is PDT+).
Whiteboard: PDT+ → [PDT+]
Whiteboard: [PDT+] → [PDT+] 12/07
Whiteboard: [PDT+] 12/07 → [PDT+] 12/09
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.)
Blocks: 21564
Priority: P1 → P3
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: 20 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 jrgm@netscape.com backlog of XPToolkits
resolved fixed bugs to verify
QA Contact: paulmac → jrgm
No longer blocks: 21564
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.