SelectorMatches is inefficient; does too many ToStrings

RESOLVED FIXED

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({perf})

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
SelectorMatches does the same ToString N times when comparing for
case-insensitive lookups.  Companion bug to 109416.  Patch factors out the
constant ToString().  This is probably 0.1 to 0.2% of pageload (SelectorMatches
is 1.5-2% total with .8% direct hits.)
(Assignee)

Comment 1

16 years ago
Created attachment 57308 [details] [diff] [review]
patch ready for review

needs r/sr
(Assignee)

Comment 2

16 years ago
Created attachment 57309 [details] [diff] [review]
correct patch

Oops, forgot to respin cvs diff after fixing a missing }.
Attachment #57308 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Status: NEW → ASSIGNED
Keywords: patch, perf

Comment 3

16 years ago
Again, would it make sense to use nsIAtom::GetUnicode instead of copying the
atom's string value?
Also, beware that the usual case is a null mIDList, and I think this patch slows
that case down.  An mIDList longer than one is extremely rare and it's not worth
optimizing for at all, so I suspect the only worthwhile change here would be to
use GetUnicode.
(Assignee)

Comment 5

16 years ago
I can easily make this patch not hurt performance for the empty-list case. 
Non-empty lists happen often enough that this shows up on dbaron's jprof.  As
you said, this doesn't help the 1-entry-list case; waterson's idea should help
with that one.

I'll provide a new patch monday.
Created attachment 58402 [details] [diff] [review]
patch with GetUnicode

Comment 7

16 years ago
Comment on attachment 58402 [details] [diff] [review]
patch with GetUnicode

r=jag
Attachment #58402 - Flags: review+

Comment 8

16 years ago
sr=hyatt
Fix checked in 2001-11-20 19:30 PDT.  And, to be honest, I doubt we hit the ID
selector matching code all that much.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.