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

NEW
Unassigned

Status

()

P5
normal
17 years ago
6 years ago

People

(Reporter: cathleennscp, Unassigned)

Tracking

(Depends on: 2 bugs, {perf})

Trunk
mozilla1.1beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [has review], URL)

Attachments

(6 attachments, 3 obsolete attachments)

(Reporter)

Description

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

similar to bug 118135

Comment 1

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

Comment 3

17 years ago
Windows lacks strtok_r.
(Reporter)

Updated

17 years ago
Target Milestone: --- → mozilla0.9.9
(Reporter)

Updated

17 years ago
Blocks: 71668
Keywords: perf
(Reporter)

Comment 4

17 years ago
Created attachment 69775 [details] [diff] [review]
eliminate nsCRT::strlen() for char * strings

eliminated 473 callers to nsCRT::strlen(char* s) and function itself.

Comment 5

17 years ago
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 )
(Reporter)

Comment 9

17 years ago
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.
(Reporter)

Comment 10

17 years ago
Created attachment 70658 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s) 

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'

Comment 12

17 years ago
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...)
(Reporter)

Comment 13

17 years ago
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));
-  }
 
(Reporter)

Updated

17 years ago
Attachment #70658 - Attachment is obsolete: true
(Reporter)

Comment 14

17 years ago
Created attachment 70750 [details] [diff] [review]
follow up on removal of nsCRT::strlen(char* s)

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 15

17 years ago
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+

Updated

17 years ago
Keywords: mozilla1.0+
(Reporter)

Updated

17 years ago
Target Milestone: mozilla0.9.9 → mozilla1.0

Comment 18

17 years ago
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+

Comment 19

17 years ago
Created attachment 76993 [details]
example to show the crash when using strlen with the parameter string null

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.

Comment 20

17 years ago
It is a foundation stability problem to encapsulat the C function 'strlen'. If
we do this, we can make mozilla more stable.

Comment 21

17 years ago
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.

Comment 22

17 years ago
Created attachment 76999 [details] [diff] [review]
patch to encapsulate the C function 'strlen'

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.

Comment 23

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

Comment 26

17 years ago
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.

Comment 28

17 years ago
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?

Comment 30

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

Comment 32

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

Comment 34

17 years ago
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.

Comment 35

17 years ago
Mmmmm... I agree with you guys. If you decide to do something more on this,
don't forget to tell me. I volunteer.
(Reporter)

Updated

17 years ago
Attachment #76999 - Attachment is obsolete: true
(Reporter)

Updated

17 years ago
Attachment #76993 - Attachment is obsolete: true
(Reporter)

Comment 36

17 years ago
Created attachment 86137 [details] [diff] [review]
final patch, remove all nsCRT::strlen(char* s)

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?
(Reporter)

Comment 38

17 years ago
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 41

17 years ago
Comment on attachment 86137 [details] [diff] [review]
final patch, remove all nsCRT::strlen(char* s)

r=dp
Attachment #86137 - Flags: review+

Updated

17 years ago
Whiteboard: [has review]
Target Milestone: mozilla1.0 → mozilla1.1beta
(Reporter)

Updated

16 years ago
Depends on: 150073
(Reporter)

Comment 42

16 years ago
Created attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

this patch eliminates nsCRT::str* call instructions, and only native str*
appears in generated assembly code.

Comment 43

16 years ago
Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

sr=darin
Attachment #117671 - Flags: superreview+

Comment 44

16 years ago
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 46

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

Comment 47

16 years ago
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 48

16 years ago
Comment on attachment 117671 [details] [diff] [review]
clean up of str* functions in nsCRT

ah ok. r=alecf
Attachment #117671 - Flags: review- → review+
(Reporter)

Comment 49

16 years ago
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.)

Comment 52

16 years ago
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.

Comment 55

16 years ago
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.

Comment 56

16 years ago
Bug 209499 is similar to this one. I have started to remove uses of PL_str*
functions. Seeking review.

Comment 57

15 years ago
Created attachment 135654 [details] [diff] [review]
ansify a few more nsCRT functions

this removes/cleans up a few methods to use ANSI C functions.

Updated

15 years ago
Attachment #135654 - Flags: superreview?(alecf)
Attachment #135654 - Flags: review?(darin)

Comment 58

15 years ago
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 59

15 years ago
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+

Comment 60

15 years ago
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? ;)

Comment 61

15 years ago
Created attachment 135660 [details] [diff] [review]
ansify a few more nsCRT functions v2

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 62

15 years ago
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.

Updated

15 years ago
Attachment #135654 - Flags: review?(darin) → review+

Comment 64

15 years ago
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.

Comment 66

15 years ago
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.

Comment 67

15 years ago
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.

Comment 69

15 years ago
>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 :)

Comment 70

15 years ago
in cvs head glibc sources, strdup does exactly that - strlen/malloc/memcpy. so i
doubt this is something to worry much about :)

Comment 71

15 years ago
Comment on attachment 135660 [details] [diff] [review]
ansify a few more nsCRT functions v2

well then, sr=alecf
Attachment #135660 - Flags: superreview?(alecf) → superreview+

Comment 72

14 years ago
*** Bug 102980 has been marked as a duplicate of this bug. ***

Comment 73

14 years ago
in /netwerk/cache still nsCRT:: strdup, and ::strcmp are used, can these also be
replaced with PL_strdup and libc:strcmp?

Comment 74

14 years ago
*** Bug 105482 has been marked as a duplicate of this bug. ***
> replaced with PL_strdup

be careful with allocators

Updated

14 years ago
Component: XP Miscellany → XPCOM
Assignee: cathleennscp → nobody
QA Contact: brendan → xpcom

Comment 76

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

Updated

6 years ago
Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.