212475 - Lazily get mEncoder in nsSegmentEncoder

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

This goes sort of hand-in-hand with bug 212223.  Even in my build that has some
optimizations for GetCharsetAlias (eg caching the service) I see a decent
speedup of nsStandardURL::SetSpec from the patch coming up...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

I put the IsAscii in the same file at jag's suggestion; I don't really see that
many other people using it.....

Comment 2

[Darin Fisher]

Comment on attachment 127590 [details] [diff] [review]
Proposed patch

>Index: netwerk/base/src/nsStandardURL.h

>+        const char* mCharset;  // Caller should keep this alive for

nit:	 |const char *mCharset;|


>Index: netwerk/base/src/nsStandardURL.cpp

>+inline PRBool
>+IsAscii(const char* str, PRUnichar len)
>+{
>+    // Here "str" is a pointer to a UTF-8 buffer and we must have len
>+    // be smaller than the length to the null terminator
>+    const char* end = str + len;
>+    while (str < end) {
>+        NS_ASSERTION(*str, "Null byte before end of data!");
>+        if (0x80 & *str)
>+            return PR_FALSE;
>+        ++str;
>     }
>+    return PR_TRUE;
> }

not too many compilers will inline a function containing
a loop.  also, i think this should be in nsCRT.  if not,
then when someone down the road needs this maybe they will
not realize that it already exists.  plus, this kind of
function deserves to be in a more generic place.

at the very least, move it into nsURLHelper.h (net_IsAscii).

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: The jag spell has worn off....

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

You could also use
  IsAscii(Substring(foo, foo+n))
if you don't want to add a new function for it.

Comment 5

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

er, IsASCII (in nsReadableUtils).

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yeah, we were semi-trying to avoid the cost of that; would that be faster than
the "scan the whole string" thing we do right now for typical URI strings?

Comment 7

[Darin Fisher]

Comment on attachment 127608 [details] [diff] [review]
The jag spell has worn off....

r=darin

i think there is good use for this nsCRT function elsewhere in the codebase.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127608 [details] [diff] [review]
The jag spell has worn off....

David, what do you think?  Would you rather I went the Substring() route?

Comment 9

[Darin Fisher]

bz:

have you compared the amount of code generated by 

  IsAscii(Substring(foo, foo+n))

and

  nsCRT::IsAscii(foo, n)

not to mention the overhead of GetReadableFragment, etc.  why should we be
creating string objects when we don't need to? ...especially given that this
code is executed A LOT.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yes, I know.  That's why I wrote it the way I did....

Comment 11

[Boris Zbarsky [:bzbarsky]]

Checked in.  This improved Tp by 1-2%, looks like.....

Comment 12

[jag (Peter Annema)]

Comment on attachment 127608 [details] [diff] [review]
The jag spell has worn off....

+  static PRBool IsAscii(const char* aString, PRUnichar aLength);

|PRUnichar aLength|???

Comment 13

[Boris Zbarsky [:bzbarsky]]

Aieee..  Total thinko.  Fixed:

-PRBool nsCRT::IsAscii(const char* aString, PRUnichar aLength)
+PRBool nsCRT::IsAscii(const char* aString, PRUint32 aLength)

and similar in the header.  Thanks for catching that, jag.

Comment 14

[benc]

Is there a whitebox/API test that can verify this change?

Comment 15

[Darin Fisher]

benc: no, not really.

Comment 16

[benc]

V
per Darin and bz' perf comments.