Closed
Bug 66577
Opened 24 years ago
Closed 24 years ago
Scheme compare changes!
Categories
(Core :: Networking, defect)
Tracking
()
VERIFIED
FIXED
mozilla0.8
People
(Reporter: gagan, Assigned: gagan)
References
Details
(Whiteboard: need sr)
Attachments
(3 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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...
Comment 3•24 years ago
|
||
When this goes in, I'll change the target= checking code and the image animation
code to use the new efficient comparison.
Comment 4•24 years ago
|
||
r=darin
Comment 5•24 years ago
|
||
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
Comment 6•24 years ago
|
||
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
Comment 8•24 years ago
|
||
Gagan: what is your solution for the subset of URI's that are unknown to
necko.so? Revert to the strcmp method?
Assignee | ||
Comment 10•24 years ago
|
||
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
Comment 11•24 years ago
|
||
r=darin on the new patch
Comment 12•24 years ago
|
||
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
Assignee | ||
Comment 13•24 years ago
|
||
I've asked pierre to add his comments to brendan's concern here.
Assignee | ||
Comment 14•24 years ago
|
||
I've checked everything except for the changes to layout/html/style since I
still haven't heard from pierre.
Comment 15•24 years ago
|
||
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)
Comment 16•24 years ago
|
||
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
Comment 17•24 years ago
|
||
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.
Comment 18•24 years ago
|
||
what about throwing in a
%{C++
#ifdef XP_OS2
#undef UNKNOWN
#endif
%}
at the top of the IDL file?
Assignee | ||
Comment 19•24 years ago
|
||
mkaply: let us know if darin's suggestion helps you. still no news from
pierre...
Comment 20•24 years ago
|
||
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?
Comment 21•24 years ago
|
||
And Gagan, sorry if I'm totally wrong with my conclusion, but I'm just trying to
learn something new hear Ok?
Comment 22•24 years ago
|
||
The idl change mentioned by darin fixes our OS/2 problem.
Comment 23•24 years ago
|
||
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.
Comment 24•24 years ago
|
||
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.
Assignee | ||
Comment 25•24 years ago
|
||
thanks for pointing out the left-overs. I added them to my list of todos for
this bug.
Assignee | ||
Comment 26•24 years ago
|
||
new patch. pls. review.
Assignee | ||
Comment 27•24 years ago
|
||
Comment 28•24 years ago
|
||
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.
Assignee | ||
Comment 29•24 years ago
|
||
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.
Comment 30•24 years ago
|
||
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
Assignee | ||
Comment 32•24 years ago
|
||
For the new suggestions I have opened bug 67866. I have done the reordering per
shaver's suggestion.
Assignee | ||
Comment 33•24 years ago
|
||
checked in.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 34•23 years ago
|
||
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.
Description
•