Closed Bug 66577 Opened 24 years ago Closed 24 years ago

Scheme compare changes!

Categories

(Core :: Networking, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla0.8

People

(Reporter: gagan, Assigned: gagan)

References

Details

(Whiteboard: need sr)

Attachments

(3 files)

A lot of consumers of nsIURI would just call getscheme to compare the scheme to a standard value. This involves an unnecessary allocation and then freeing of the scheme string. This should be changed to have a scheme compare funtion on nsIURI. I already have this done, but I can't find the bug number where this was originally written. Maybe I just dreamed of it. Anyhoo...
cc'ing people who maybe interested in this.
Target Milestone: --- → mozilla0.8
When this goes in, I'll change the target= checking code and the image animation code to use the new efficient comparison.
r=darin
Purty,.and uri->SchemeIs("http", &isHTTP) is even more readable than the Java-style IsScheme or my HasScheme. I hope mscott won't mind my offering r/sr=. One nit: "and thereby" can be just "thereby" in the doc-comment for schemeIs in nsIURI.idl. /be
I don't mind at all Brendan help yourself!
thanks for that quick review there brendan. I made this even more optimized than string compares and so a new patch is coming up. The big change here is that I am using enums for standard URI types and hence the comparison is now reduced to that of an int. this should really speed things up significantly.
Status: NEW → ASSIGNED
Gagan: what is your solution for the subset of URI's that are unknown to necko.so? Revert to the strcmp method?
darin: yes. there is a default type which is nsIURI::UNKNOWN and if that is the case then the SchemeIs function would fail, thereby allowing clients to write their comparisons just the way they are (before this patch) i.e. with getscheme. Note that this patch does not anyway affect the underlying strings/specs. So anyone using the old style comparison routine will not have any problems. This patch only optimizes for the known cases of-- about, chrome, file, ftp, http, https, imap, jar, javascript, mailbox, mailto, news, and resource. So who's giving me a r/sr on the newer patch? TIA
r=darin on the new patch
Whiteboard: need sr
sr=brendan@mozilla.org, but get pierre's consent before checking into layout/html/style, ok? If he thinks your patch is too scary to think about right now, send him my way. /be
I've asked pierre to add his comments to brendan's concern here.
I've checked everything except for the changes to layout/html/style since I still haven't heard from pierre.
Can I ask why this were coded as all capital enums? It seems like that is just begging to conflict with a #define which it did on OS/2 (UNKNOWN)
ALL_CAPS is Mozilla, JS, and Java style for constants. xpidl generates a singleton enum scoped by the interface's class for each such constant. I doubt there will be #define conflicts with system headers, because apart from very old macros such as SEEK_SET, EOF, and a relative handful of other well-known names, no system header included by Mozilla code should preempt UNKNOWN, HTTP, or JAVASCRIPT. Is there a real problem here? /be
We actually did break on OS/2. Our math.h #defines UNKNOWN to 7. Right now I have the tinderbox math.h hacked to remove it. I'm not terribly concerned, because we've had to hack up our system headers already anyway (see the first steps at http://www.mozilla.org/ports/os2/vacppbuild.html) If this is one more thing we have to tell people to modify on their system, that would be OK. I just wanted to understand the situation better.
what about throwing in a %{C++ #ifdef XP_OS2 #undef UNKNOWN #endif %} at the top of the IDL file?
mkaply: let us know if darin's suggestion helps you. still no news from pierre...
Gagan, can it be that bug 67347 run into trouble with your recent patch? This code snap from a testcase: <a href="javascript:document.location.reload();">Reload his page through a direct href call</a> now no longer work! It looks like a regression after you checkin, can this be true?
And Gagan, sorry if I'm totally wrong with my conclusion, but I'm just trying to learn something new hear Ok?
The idl change mentioned by darin fixes our OS/2 problem.
I've confirmed H-J's suspicion. Looks likethe call to schemeIs at nsDocShell.cpp:3331 is returning false when it should be true. A javascript: URI uses nsSimpleURI as its implementation, and nsSImpleURI's mSchemeType is set to UNKNOWN (0), and apparently never changed to JAVASCRIPT (9). Gagan, I defer to you on the best way to fix this. Should nsSimpleURI look at its scheme string on initialization and set mSchemeType accordingly? Or does there need to be an nsJSURI? I think the former is best, but you may have a better idea altogether.
There are some left-over in the "nsDocShell: Site Loading" function, which seems to be called heavily: 3034 NS_IMETHODIMP nsDocShell::InternalLoad(nsIURI* aURI, nsIURI* aReferrer, ... 3048 nsresult rv = aURI->GetScheme(getter_Copies(scheme)); 3049 if (NS_SUCCEEDED(rv) && 3050 nsCRT::strcmp(scheme, "chrome") && 3051 nsCRT::strcmp(scheme, "resource")) There is another left-over in the final nsDocShell::DoURILoad() as well.
thanks for pointing out the left-overs. I added them to my list of todos for this bug.
new patch. pls. review.
Gagan, Could you please add the "data" protocol to your comparison function? Also, and this is a nit, but would it improve the performance of the SchemeTypeFor function by a fraction if you put the most common schemes higher on the list? For example, putting "http" first would catch a lot of URLs with only one comparison.
Mitch I really liked the idea of moving the more frequent ones up, but I am not convinced that data is that frequent a check that we need to optimize for.
r/sr=brendan@mozilla.org, let's get this in today. mstoltz, can you stamp an r= here? Maybe if gagan opens a new perf bug to investigate whether ordering the ifs in SchemeTypeFor helps performance noticably? /be
Keywords: mozilla0.8
I agree that we should look at moving more frequent ones to the top of the list, especially chrome: and resource:, where we're not going to be waiting for DNS traffic. PL_strcasecmp breaks my heart, but what can we do? gperf could cough up a minimal perfect hash for us, or we could teach NSPR to use the system strcasecmp instead of its naive version. But I'll not hold my sr= for that, in the face of broken javascript:. If you reorder the final comparison as we discussed in IRC, sr=shaver.
Keywords: mozilla0.8
For the new suggestions I have opened bug 67866. I have done the reordering per shaver's suggestion.
checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
QA Contact: tever → bbaetz
VERIFYING - lxr shows that this was checked in.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: