Open Bug 124536 Opened 21 years ago Updated 8 months ago

Eliminate nsCRT::str* to use str* from libC

Categories

(Core :: XPCOM, defect, P5)

defect

Tracking

()

mozilla1.1beta

People

(Reporter: cathleennscp, Unassigned)

References

(Depends on 2 open bugs, )

Details

(Keywords: perf, Whiteboard: [has review])

Attachments

(6 files, 3 obsolete files)

removes levels of indirections in code that we don't need, to improve performance.

similar to bug 118135
cathleen:
nsCRT::strtok() should be replaced with PL_strtok_r() (same function as
strtok_r()) ...
Why do we need to use PL_strtok_r?  Does any platform lack strtok_r?

/be
Windows lacks strtok_r.
Target Milestone: --- → mozilla0.9.9
Blocks: 71668
Keywords: perf
eliminated 473 callers to nsCRT::strlen(char* s) and function itself.
Comment on attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

r=dp
Attachment #69775 - Flags: review+
Comment on attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

Go strong!  Don't let any more nsCRT::strlen on char * data go in while you
blink!

/be
Attachment #69775 - Flags: superreview+
This caused mac/beos bustage because of bug 102980 - if size_t is unsigned long,
rather than unsigned int, then the overloading between the two versions of
nsCRT::strncmp we have is ambiguous.

I imagine that that bug will be fixed for char* when you use the native strncmp,
though, although it won't fix the prunichar* case.
nsCRT.h still includes a comment for the now-removed nsCRT::memcpy function,
maybe you can remove it while you're at the file.
( http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#111 )
Comment on attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

I landed all changes in this patch (for last 2 days), except removing actual
nsCRT::strlen(char* s) function itself.

A new patch to flow up for diff left to land.
remove nsCRT::strlen(char* s)and fixed up comments in nsCRT.h
added back changes that was erased by taka's check-ins in mailnews.
Comment on attachment 70658 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s) 

>+   ***  In addition, the follwoing char* string utilities
>+   ***  are no longer supported either.  Plse use lib C also.

Typo for 'following' and 'please'
just a reminder that you can't whack strcasecmp/strncasecmp or strdup.

   /** Compute the string length of s
    @param s the string in question
    @return the length of s
    */
-  static PRUint32 strlen(const char* s) {
-    return PRUint32(::strlen(s));
-  }
 
also, please don't remove a function without removing its comment. doing so 
will mess up things like 
http://unstable.elemental.com/mozilla/build/latest/mozilla/xpcom/dox/classnsCRT
.html (or humans trying to read the code...)
I will remove this comment, when the last str* function is removed.  Right now,
I plan to leave it in till then.  :-)


 /** Compute the string length of s
    @param s the string in question
    @return the length of s
    */
-  static PRUint32 strlen(const char* s) {
-    return PRUint32(::strlen(s));
-  }
 
Attachment #70658 - Attachment is obsolete: true
same as attachment 70658 [details] [diff] [review], fixed typos in comments :-)

remove nsCRT::strlen(char* s) and updated comments in nsCRT.h
added back changes that was erased by taka's check-ins in mailnews.
Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

r=dp
Attachment #70750 - Flags: review+
Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

sr=brendan@mozilla.org
Attachment #70750 - Flags: superreview+
Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

a=roc+moz for 0.9.9
Attachment #70750 - Flags: approval+
Keywords: mozilla1.0+
Target Milestone: mozilla0.9.9 → mozilla1.0
Comment on attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

removing approval. if this still needs to land please re-request approval from
drivers. thanks.
Attachment #70750 - Flags: approval+
After examing the usage of 'strlen' for some time, I think it is a better way
to encapsulate the function. Because if we pass a parameter of NULL to
'strlen', the application will crash. The attachment I give today(04/01/2002)
is an example to show the crash. I've verified this on both Win2k and Solaris.
And I find that in mozilla a lot of people doesn't check the parameter before
passing it in. So this is a very serious implied problem.
I'm not sure if I should file a new bug or keep on working on this bug, so I
just discuss this issue here first. If you think I should file a new bug, just
tell me.
BTW: I'll try to give a patch for the encapsulation with almost no performance
lost soon, so this issue may be discussed more clearly and quickly.
It is a foundation stability problem to encapsulat the C function 'strlen'. If
we do this, we can make mozilla more stable.
I agree with henry, just like NSPR
we use NSPR, will lose some perf, but at the same time, 
we can use it more safely.
This is the patch for encapsulating the C function 'strlen'. Using the
encapsulating function 'nsCRT::strlen' instead of directly using the C function
'strlen' will cause almost no performance lose in average. Although it will add
a comparing operation, the operation will use very little time and when the
parameter string is NULL, there is no need to call the C function and directly
return 0.
If this patch can be r= and sr=, we need to go back to reuse the original
nsCRT::strlen instead of direct strlen. So although I strongly suggest to
encapsulating 'strlen', we must think this issue clearly. If we need to reuse
nsCRT::strlen, I volunteer to do this work.
BTW: Other similliar functions, such as str*, printf, etc, also need to be
taken into account.
By using NSPR, we can not only make mozilla cross-platform, but also give out a
set of APIs which are more stable and more easy to use.
NSPR does not wrap ("encapsulate") all Standard C and C++ functions, nor should
it.  Besides defeating optimizations where the compiler can disambiguate strlen
and inline it appropriately, such wrapping adds cost.  We do not know how high
that cost is without measuring real-world benchmarks with and without wrapping.

Then there is the question of null-testing char * or const char * in parameters.
 If we had been null-testing the parameter to nsCRT::strlen(const char* s) for
years, then I would agree that we can't stop checking for null.  However,
nsCRT::strlen has *never* null-tested s.  The standard C function strlen does
not null-test either.  So I claim we should not keep nsCRT::strlen, because
(1) we do not want any overhead of wrapping, however small it appears without
measurement;
(2) we do not want any overhead of null-testing, same reason;
(3) we do not need null-testing for nsCRT.h "API compatibility", and now is not
the time to be changing the API semantics.

Let's not actually encourage callers to be sloppy about null vs. a valid char
pointer.  A C string is referenced via a (const or not) char *, which is never
null.  The C standard doesn't allow null as a valid parameter to strlen, and
neither should Mozilla allow it in any strlen-like wrapper, or pass null into
anything named strlen.

/be
What's more, all this talk about "stability" and "portability" is conjecture. 
We have code that is highly portable already.  Until someone can demonstrate a
real world portability problem that requires wrapping strlen or other standard C
library functions, we should do nothing.  To do the opposite in the name of some
abstract good such as stability or portability is to bloat code and add cycle
costs without reason.

/be
Henry Jia:
If someone hits the "|strlen(NULL)|-causes-crash"-issue in a production build
he/she can use |% export LD_PRELOAD=/usr/lib/0@0.so.1; ./mozilla| in Solaris to
work around these crashes (0@0.so.1 maps a zero-filled page at the address 0 to
make it a valid address) ...
If someone finds strlen(NULL) being called, then they can file a bug so that the
code cam be fixed.
Anyway, I still stick to my opinion for the following reasons.
1. PL_strlen wrap strlen with the null parameter checking
(http://lxr.mozilla.org/seamonkey/source/nsprpub/lib/libc/src/strlen.c#40). And
it still be used in the trunk code. Since we're trying to eliminate it, we
should pay attention to the places in which it is used.
2. I still think by wraping strlen, we can make mozilla programming more easy
and safe.
I'm against using PR_strlen, it's much slower than simply using strlen(),
because a) it's not inlined and b) it does the null check

For several years we have not null checked strings passed into strlen, why
should we start now?
I think we should file a bug to get PL_strlen() removed from NSPR code.
|strlen()| is part of ANSI-C and there is AFAIK no need to keep it around...

brendan ?
As wtc wrote elsewhere, NSPR has been around for many years, and it keeps
compatibility according to some strict rules.  It's not about to remove the
PL_str* wrappers, on that account, but they are deprectated, and there is *no
good reason* to use them where standard C provides working counterparts, and
where no one passes in null.

/be
Brendan wrote:
> and there is *no good reason* to use them where standard C provides 
> working counterparts, and where no one passes in null.

Did you ever make a search how many people use PL_strlen() ? I think we should
put a LARGE comment (like "do not use - deprectated") into the header file and
source because some people still think that PL_strlen() is mandatory... ;-(
Roland: sorry I misspelled "deprecated" (a typo), please don't repeat my
mistake.  You might file a bug on NSPR asking for deprecation comments, but wtc
is cc'd here, so let's ask him first.

wtc, what do you say to deprecation comments?

/be
Sure.  We should go one step further and implement PL_strxxx as a call
to strxxx if strxxx is a standard C library function.  This will reduce
the performance penalty of PL_strxxx to function call overhead and the
inability of inlining by compilers.
Mmmmm... I agree with you guys. If you decide to do something more on this,
don't forget to tell me. I volunteer.
Attachment #76999 - Attachment is obsolete: true
Attachment #76993 - Attachment is obsolete: true
attatchment 70750 had landed back in Feb (for 0.9.9).

This new patch cleans up all final traces and creep backs, over last 3 mo.
Shouldn't the patch also remove the nsCRT::strlen function itself, to ensure
that it's not used anymore?
yes, of course. removing nsCRT::strlen(char* s) is part of the latest patch.
Comment on attachment 86137 [details] [diff] [review]
final patch, remove all nsCRT::strlen(char* s)

sr=brendan@mozilla.org

/be
Attachment #86137 - Flags: superreview+
oh indeed.
but it leaves the comment about the function's usage in, shouldn't that be
removed as well?
Comment on attachment 86137 [details] [diff] [review]
final patch, remove all nsCRT::strlen(char* s)

r=dp
Attachment #86137 - Flags: review+
Whiteboard: [has review]
Target Milestone: mozilla1.0 → mozilla1.1beta
Depends on: 150073
this patch eliminates nsCRT::str* call instructions, and only native str*
appears in generated assembly code.
Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

sr=darin
Attachment #117671 - Flags: superreview+
Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

alec: can you review this.  thx!
Attachment #117671 - Flags: review?(alecf)
The nspr strcmp is null-safe, whilst the libc ones (and thus these replacements)
aren't. Hopefully noone tries to pass a null string in, though.
Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

can't we use configure to check for stricmp and friends? I'm concerned that 
XP_UNIX won't be enough... a simple one or two line check in configure.in,
something like 
AC_CHECK_FUNCS(stricmp, strnicmp)
Attachment #117671 - Flags: review?(alecf) → review-
alec:

i originally wrote some configure.in checks, but they weren't being executed on
windows... asked seawood... and he said, use an #ifdef in the header file.
turns out most of the checks we run in configure.in are disabled for WIN32.
Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

ah ok. r=alecf
Attachment #117671 - Flags: review- → review+
yeah, darin said it.  :-) thanks guys! :-)  
bbaetz: you mean null pointer, not null string -- the latter is ambiguous, often
meaning an empty string.  Null-the-pointer (0 in a pointer context) is not a
valid C string (char * or const char *) value.  I hope we can safely assert this
in all of our code!

/be
Just for clarification, I said that there's no need to add a configure check for
strlen because it's in the ANSI C standard and we're already using it (w/o
nsCRT:: or PL_) in numerous places in the code.  Unless we have reason to
believe strcasecmp isn't available on some platform, I wouldn't bother with a
check there either.  I'm guessing from the patch that it's not available on
win32 & os2 so we probably should add a check for it. (Not that we should hold
the patch for it.)
so, there are some hangups with this patch.  namely, linux (RH7.3) doesn't
provide strcasecmp or strncasecmp unless __USE_BSD is defined above #include
<string.h>.  that means we are going to need some configure.in magic... but i
worry that setting such defines (maybe _BSD_SOURCE is equivalent) globably would
have some side effect?

also, bbaetz's point is very good... i don't know if we want to bite the bullet
and risk unprotected strcmp.  we could just implement nsCRT::strcmp like this:

  PRInt32 strcmp(const char *s1, const char *s2)
  {
    if (!s1 || !s2)
      return PRInt32(s1 - s2);

    return ::strcmp(s1, s2);
  }

but now this might end up increasing code size :(

thoughts?
brendan: Yeah, thats what I meant. I hope we can assert that too, but given how
long these have tested their arguments....

The compielr should be bale to optimise the inlined null check out when it can
prove that the result isn't null. I'm happy with it as is, but we could assert
and test for a few weeks if others aren't, and see if anyone gets caught. I jsut
wanted to mention it...
I think the only adverse side-effect -D_BSD_SOURCE would have is to enable a
bunch of annoying warnings.  That was the reason we started using _POSIX_SOURCE
way back when.  We could just remove the *_SOURCE defines from configure.in and
have all of the default glibc API (which includes bsd routines) available.
seawood: yeah, there are several places where we have to add -D_BSD_SOURCE to
build specific files: xpcom/io, netwerk/base/src, netwerk/cache/src.  ftruncate
being one such system call that we use in necko.  if you are ok eliminating
_POSIX_SOURCE, then i think we should just do that.  it would greatly simplify
using the extended functions.
Bug 209499 is similar to this one. I have started to remove uses of PL_str*
functions. Seeking review.
this removes/cleans up a few methods to use ANSI C functions.
Attachment #135654 - Flags: superreview?(alecf)
Attachment #135654 - Flags: review?(darin)
oh, regarding the comment in strncasecmp - i checked all the usages in the tree,
and no one's abusing the return value. it seems that comment was added at the
same time as an identical one in xpcom/string, so it probably has no significance.

i also don't see what it means by "very negative numbers". PL_strncasecmp will
return the difference between two |char| values when the strings differ, so it
could at most be -128.
Comment on attachment 135654 [details] [diff] [review]
ansify a few more nsCRT functions

did you try compiling with this? I feel like we're using some of the
nsCRT::strncmp stuff in templates..i.e. so you can apply it in

template<T>
int MyCompare(T* a, T* b) {
return nsCRT::strncmp(a, b, 100);
}

I tried to remove these once before and I think it was darin who objected for
that reason

anyway, sr=alecf on all the non-inlined stuff

for inline stuff like nsCRT::strncmp() which is just a direct forward to the
libc version, I don't feel like they're doing much harm (But maybe we still
need to update the comment there!)
Attachment #135654 - Flags: superreview?(alecf) → superreview+
yup, i did build with this... but egads, looking at it now, there's a pretty
nice mistake(?) in the current nsCRT.h, and in my patch...

http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#144
http://lxr.mozilla.org/seamonkey/source/xpcom/ds/nsCRT.h#164

so we have two of the same method, with different types of the integer arg! that
can't be right...

anyway, what would eventually be nice to do is make nsCRT a home for libc
wrappers that aren't available in ANSI C (like cathleen tried to do for
strcasecmp here). then we could start removing usages of the (inherently much
slower) PL_* functions that these replace, such as PL_strcasecmp.

eliminating the ANSI C wrappers we have now would also be nice, especially since
they call PL_str* versions which are inherently slower. those consumers that
explicitly need nullsafety could either add the checks themselves, or use the
PL_str* versions. but that's not too big of a deal.

anyway, which comment were you referring to alec? ;)
remove the duplicate fn... took the safe route and removed the non-nullsafe
one.

i'll fix the callers of this method to do the right thing at some other point.
Comment on attachment 135660 [details] [diff] [review]
ansify a few more nsCRT functions v2

alec, could i get you to sr this again? :/
Attachment #135660 - Flags: superreview?(alecf)
Be very careful. Thats not a mistake, its on purpose, it appears. See bug 102980.
Attachment #135654 - Flags: review?(darin) → review+
hmm... nice. thanks bbaetz. so, my build compiles fine with the duplicate
function removed... are there any platforms that might break (i.e. if a consumer
was trying to pass a PRInt32, and the compiler decides the remaining PRUint32
prototype isn't compatible)?

otherwise i think we *should* be able to remove it... so my sr request still stands.
Since we no longer need to worry about Mac OS9, you should also be able to ditch
PL_strdup in favor of strdup, I think.
Have you checked that every platform has strdup? It is not part of the C
standard, at least not C90. I'd expect that some of the older UNIX platforms
lack it. strcat, strcmp, etc are good functions to use directly, but in general
strdup is not.
i'm not too fussed about strdup... my patch breaks PL_strdup down into a
strlen/malloc/memcpy anyway, and the malloc is slow, so it's not perf-critical :)

if we wanted to do something like that, we'd probably have to go for a configure
test.
Aaron: as I remember it, when people used to check in code with strdup() in it,
Mac OS9 tinderboxen were the only ones that broke.  It's possible that I'm
misremembering, however.  As dwitte points out, an autoconf test would be ideal.
>i'm not too fussed about strdup... my patch breaks PL_strdup down into a
>strlen/malloc/memcpy anyway, and the malloc is slow, so it's not perf-critical :)

incidentally, dbaron mentioned that he compared GLIBC's strdup against
strlen+malloc+memcpy and found the latter to be faster!  the reason, he said,
had to do with strdup copying the data byte-by-byte.  memcpy on the other hand
copies data one WORD at a time (modulo special handling at the beginning and
end).  of course, this may have been fixed in more recent GLIBC versions :)
in cvs head glibc sources, strdup does exactly that - strlen/malloc/memcpy. so i
doubt this is something to worry much about :)
Comment on attachment 135660 [details] [diff] [review]
ansify a few more nsCRT functions v2

well then, sr=alecf
Attachment #135660 - Flags: superreview?(alecf) → superreview+
*** Bug 102980 has been marked as a duplicate of this bug. ***
in /netwerk/cache still nsCRT:: strdup, and ::strcmp are used, can these also be
replaced with PL_strdup and libc:strcmp?
*** Bug 105482 has been marked as a duplicate of this bug. ***
> replaced with PL_strdup

be careful with allocators
Component: XP Miscellany → XPCOM
Assignee: cathleennscp → nobody
QA Contact: brendan → xpcom
See bug 181295 for discussion on PL_strdup (that needs to match PL_strfree).
Depends on: 743581
Filing a blocking bug to replace the nsCRT::strcmp with the libc::strmcp
Depends on: 791546
Depends on: 798840
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.