27971 - res protocol snapshots substitutions so aggressively

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[Warren Harris]

I discovered tonight some of my own braindead code. Whenever we make a resource 
channel, we snapshot the list of substitutions that it can perform so that the 
protocol handler is free to add or delete substitutions after that point. But 
this is wasteful because substitutions probably hardly ever change.

A better solution would be to refcount the substitution list, letting clients 
share it. Then if the protocol handler wants to update it, it can drop its 
copy, and make a clone with the new element inserted. This will allow the list 
to usually be shared, cutting down on resource: resolution time and memory 
consumption.

This gets called for every chrome url that gets resolved, every resource loaded 
from every xul file, etc. so it would be good to fix.

This may require implementing some refcountable verion of nsCStringArray 
though.

Comment 1

[Warren Harris]

Moving non-essential, non-beta2 and performance-related bugs to M17.

Comment 2

[Warren Harris]

post-beta2

Comment 3

[Warren Harris]

I see nsResChannel showing up as a significant contributer on Rusty's table 
bloatblame logs, and I think fixing this bug will help the situation 
considerably.

I'm going to submit a patch shortly. Can one of you review it?

Comment 4

[Warren Harris]

Attachment: resource: protocol optimization, fix for NS_ConvertUCS2toUTF8 and chrome registry

Comment 5

[Warren Harris]

Here's the deal on this patch:

1. Added Clone method to nsISupports array for convenience.
2. Fixed nsString's NS_ConvertUCS2toUTF8 method to work.
3. Fixed nsStdURL::SetFile to deal with the fact that nsIFile doesn't put 
trailing slashes on directories.
4. Changed nsIResProtocolHandler::GetSubstitutions to return an 
nsISupportsArray of nsIURIs rather than a string array.
5. Changed substitution lists to contain urls instead of strings, and made them 
copy on insertion, rather than copy on access (much better since the ratio of 
insertion to access is low).
6. Fixed nsResChannel to deal with new representation of substitutions.
7. Cleaned up "special directory" to use new directory service (nsIFile-based) 
stuff.
8. Fixed chrome registry to deal with null chrome data source (this seemed to 
be happening for me on startup -- not sure why, but could be related to the 
NS_ConvertUCS2toUTF8 bug).

I'll try bloatblame tomorrow to see how this fares. Let me know if these diffs 
look ok so I can check them in.

Comment 6

[Chris Waterson]

For NS_ConvertUCS2toUTF8, what we want to do is *not* reinitialize utf8len in 
the second loop. Incrementing it as you do now will cause it to count the number 
of UTF-8 characters, but not accurately reflect the length of the string in the 
case of a multi-byte character.

Comment 7

[Warren Harris]

Well, if you don't increment there, the length always comes out 0.

Comment 8

[Chris Waterson]

There are two loops: uft8len is computed in the first loop, and then (dumb
copy-n-paste error) re-initialized in the second.

Comment 9

[Warren Harris]

Checked in. Looks like it saved about .4% of the entire run just bring up one 
page (www.mozilla.org) -- about 60k. I think that's significant.

You can see the raw data here:

before: 
ftp://btek/pub/before-fun.html#nsResChannel::EnsureNextResolvedChannel(void)

after:  
ftp://btek/pub/after-fun.html#nsResChannel::EnsureNextResolvedChannel(void)

Comment 10

[Tom Everingham]

verified