Closed Bug 394442 Opened 18 years ago Closed 18 years ago

Optimize ID, class and style getters

Categories

(Core :: DOM: Core & HTML, enhancement)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
All
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(2 files, 1 obsolete file)

patch
7.01 KB, patch
bzbarsky
: review+
sicking
: superreview+
Details | Diff | Splinter Review
initilize prototype members
16.24 KB, patch
sicking
: review+
sicking
: superreview+
jst
: approval1.9+
Details | Diff | Splinter Review
Attached patch patch — — Splinter Review
When matching the style for an element, there are lots of GetID, GetClasses and GetInlineStyleRule calls, and pretty often (at least when running for example tdhtml or tp2) there isn't ID, class or style set at all. There are plenty of bits left in nsINode::mFlagsOrSlots, so those could be used to optimize this a bit, basically to remove mAttrsAndChildren.GetAttr calls. On my machine the patch seems to improve tdhtml and tp2 slightly, though noise is quite bad. One could argue that the patch is over-optimization but maybe still worth to try and then back it out if it doesn't improve anything.
Comment on attachment 279090 [details] [diff] [review] patch Bz, do you think this is worth to try?
Attachment #279090 - Flags: review?(bzbarsky)
Comment on attachment 279090 [details] [diff] [review] patch Looks ok. I don't like that castnig all that much, but....
Attachment #279090 - Flags: review?(bzbarsky) → review+
Comment on attachment 279090 [details] [diff] [review] patch Yeah, that cast shouldn't be needed. Also, shouldn't you fix nsXULElement since that does not yet inherit nsStyledElement. Although it could be argued that that should be changed intstead. with that fixed, sr=me
Attachment #279090 - Flags: superreview+
Sure the const_cast is needed, and I think it didn't compile without the static_cast. I could fix XULElement too, and also SVG (class), but I'd like to try first this simple patch to test whether this improves anything on tboxes. (tp and tdhtml are pretty much html only tests) Especially XUL needs a bit more changes, because of prototypes. That could be a followup patch.
I test-landed the patch and it did seem to improve tp/tp2 a bit. However there was a mochitest failure; <html id="some_id"> is handled somehow differently to other elements. Haven't yet found what the problem is. testcase, http://www.mozilla.pettay.fi/moztests/html.html
So the problem is that ::GetID is used before the attribute has been set, so the flag is removed. Need to figure some reasonable way to fix that, or just not remove the flag ever.
This time passes mochitest. If the patch is ok, approval for 1.9 would be great too ;)
Assignee: nobody → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #279366 - Flags: superreview?(jonas)
Attachment #279366 - Flags: review?(jonas)
Comment on attachment 279366 [details] [diff] [review] never unset flags, optimize also XUL >+ PRPackedBool mHasIdAttribute:1; >+ PRPackedBool mHasClassAttribute:1; >+ PRPackedBool mHasStyleAttribute:1; These should be ofc initialized to PR_FALSE in the constructor.
Attachment #279366 - Attachment is obsolete: true
Attachment #279629 - Flags: superreview?(jonas)
Attachment #279629 - Flags: review?(jonas)
Attachment #279366 - Flags: superreview?(jonas)
Attachment #279366 - Flags: review?(jonas)
btw, when starting FF this saves (IIRC) ~8000 FindLocalOrProtoAttr calls. That might be noticeable.
Maybe not blocking 1.9 but wanted at least. This should help with tp/tp2 a bit.
Flags: blocking1.9?
Comment on attachment 279629 [details] [diff] [review] initilize prototype members The nsXULContentSink.cpp change looks unrelated. r/sr=me with that fixed.
Attachment #279629 - Flags: superreview?(jonas)
Attachment #279629 - Flags: superreview+
Attachment #279629 - Flags: review?(jonas)
Attachment #279629 - Flags: review+
(In reply to comment #12) > (From update of attachment 279629 [details] [diff] [review]) > The nsXULContentSink.cpp change looks unrelated. It is needed, because I changed the type of mScriptTypeID
Attachment #279629 - Flags: approval1.9?
Attachment #279629 - Flags: approval1.9? → approval1.9+
Checked this in, but unfortunately improved perf only very slightly. Not sure why I saw bigger change when test-checked-in the first patch.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Flags: blocking1.9?
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: