84194 - Patch that makes color parsing faster

Status: RESOLVED FIXED

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

Description

[Daniel Bratell]

The attached patch makes color parsing twice as fast. It doesn't matter on pages 
beside the one in the URL which has 10000 colors specified, but anyway, it makes 
that page load about 1% faster.

Comment 1

[Daniel Bratell]

Attachment: Patch to make color parsing faster

Comment 2

[Johnny Stenback (:jst)]

Thanks for the patch, I'll let Harish deal with this one.

One comment about the patch tho:

+  if (aString.Length() == 0) {
+    return PR_FALSE;
+  }

should use if (aString.IsEmpty()).

I'm glad to sr= this once Harish has a look at this (and once I actually *read*
the patch).

Comment 3

[Blake Ross]

Harish, can you take a look at this patch so we can get it in?  No sense in 
letting it go to waste. Thanks.

Comment 4

[harishd]

Nit pick: nscolor declaration, i.e. nscolor color=0, could be delayed further.

And with suggested change from Johnny r=harishd.

Comment 5

[Daniel Bratell]

I've done the two changes mentioned, but looking at this again, I see an even 
greater optimization. No color name can start at '#', right? So we should look 
at the first char, and if it's '#' do the numerical conversion. 

I will attach a patch that does that in a little time.

Comment 6

[Daniel Bratell]

Attachment: Updated patch

Comment 7

[Daniel Bratell]

Attachment: The updated patch for _this_ bug. Forget the last attachment.

Comment 8

[Daniel Bratell]

This was better. Quantify numbers for 10000 calls:

In the code today      72.1 ms (1.73% of the total time)
The last patch         16.3 ms (0.43%)

(The percentages are approximate since I played with other stuff affecting the 
total time for the page).

harishd, can you take a new look? 
jst, I think this is mature enough for a real sr?

Comment 9

[harishd]

looks good to me. r=harishd.

Comment 10

[Johnny Stenback (:jst)]

Looks good to me, sr=jst

Comment 11

[Blake Ross]

Worth getting into .9.2/limbo?

Comment 12

[harishd]

Blake: I would say so.

Comment 13

[harishd]

Blake: I would say so.

Comment 14

[Daniel Bratell]

I finally noticed that " " (SPACE) is missing from the set of whitespace chars. 
That's fixed in my tree now.

Comment 15

[Daniel Bratell]

Attachment: Updated patch with SPACE stripping

Comment 16

[Daniel Bratell]

harsihd, jst, here is a new patch that also strips SPACE. Can I have a new r/sr?

Comment 17

[Johnny Stenback (:jst)]

sr=jst

Comment 18

[harishd]

r=harishd. 

Daniel: since you did all the work...reassigning bug to u :-)

Comment 19

[Daniel Bratell]

Fix checked in

Comment 20

[Daniel Bratell]

...and the bug is therefore FIXED.