res protocol snapshots substitutions so aggressively

VERIFIED FIXED in M18

Status

()

Core
Networking
P3
normal
VERIFIED FIXED
18 years ago
18 years ago

People

(Reporter: Warren Harris, Assigned: Warren Harris)

Tracking

({perf})

Trunk
All
Other
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

18 years ago
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.
(Assignee)

Updated

18 years ago
Keywords: perf
Target Milestone: M15
(Assignee)

Comment 1

18 years ago
Moving non-essential, non-beta2 and performance-related bugs to M17.
Target Milestone: M15 → M17
(Assignee)

Comment 2

18 years ago
post-beta2
Target Milestone: M17 → M18
(Assignee)

Comment 3

18 years ago
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?
(Assignee)

Comment 4

18 years ago
Created attachment 9817 [details] [diff] [review]
resource: protocol optimization, fix for NS_ConvertUCS2toUTF8 and chrome registry
(Assignee)

Comment 5

18 years ago
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

18 years ago
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.
(Assignee)

Comment 7

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

Comment 8

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

Updated

18 years ago
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

18 years ago
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

18 years ago
verified
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.