112209 - ns*String should be only consumers of nsStr

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Alec Flett]

in an effort to rid ourselves of nsStr, we need to make sure nobody is actually
using nsStr outside of the string code.

I've found a few occurances, I'll attach patches as I make them

Comment 1

[Alec Flett]

adding scc and dbaron, for reviews

Comment 2

[Alec Flett]

Attachment: convert nsStaticNameTable over to nsDependentString

nsStaticNameTable was the only consumer of nsStr::Initialize, and it was using
it to create an nsCString that didn't own its own buffer... which is what
nsDependentString is for.

So, I created a buffer of nsDependentCStrings and used placement-new to
initialize the buffer. I had to update the callers as well.

Comment 3

[Alec Flett]

after that patch, there are only 2 more consumers, very little research done here:
nsBulletFrame.cpp which uses nsStr::Delete to remove one character
nsMsgCompse.cpp, which also uses nsStr::Delete to remove clumps of characters.

Comment 4

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

Comment on attachment 59355 [details] [diff] [review]
convert nsStaticNameTable over to nsDependentString

>Index: xpcom/ds/nsStaticNameTable.h
>+  const nsDependentCString& GetStringValue(PRInt32 index);

I'd prefer if this returned |const nsAFlatString&| rather than
|const nsDependentString&| so that we'd have a little more freedom
to change the implementation in the future.  (You'd also have to
fix all the places you did |const nsDependentString& foo = ...|.)

>Index: xpcom/ds/nsStaticNameTable.cpp
>+    nsMemory::Free((void*)mNameArray);

You might at least want to leave a comment that you're assuming
that the destructor of nsDependentString is empty.

>        NS_ASSERTION(temp1.Equals(temp2), "upper case char in table");

I slightly prefer |temp1 == temp2|, but it's your call.

>Index: layout/html/base/src/nsTextTransformer.cpp

It would be much simpler if you just did this work from the layout
module destructor rather than having your own observer.  If, when
(if?) we start unloading DLLs, we find that the module destructors
are doing some of these things too late, we could have one XPCOM
shutdown observer per-module and move some of the things from the
module destructor into it.


>Index: content/shared/public/nsCSSKeywords.h
>+  static const nsDependentCString& GetStringValue(nsCSSKeyword aKeyword);

Again, I'd prefer |const nsAFlatCString&| to |const nsDependentString&|.

>Index: content/shared/public/nsCSSProps.h
>+  static const nsDependentCString& GetStringValue(nsCSSProperty aProperty);

Again.

>+  static const nsDependentCString& LookupPropertyValue(nsCSSProperty aProperty, PRInt32 aValue);

And again.

>+  static const nsDependentCString& SearchKeywordTable(PRInt32 aValue, const PRInt32 aTable[]);

And again.

Comment 5

[Alec Flett]

oh, that nsTextTransformer thing wasn not supposed to be in there! that's bug 112190

I'll add calling the destructor, and convert everyone to nsAFlatCString

Comment 6

[Alec Flett]

Attachment: patch addressing dbaron's comments

- manually call the destructor
- changed nsDependentCString to nsAFlatCString

Comment 7

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

Comment on attachment 59362 [details] [diff] [review]
patch addressing dbaron's comments

r=dbaron

I just noticed one more thing, though -- |IsNullString| is unused, and it
doesn't seem too meaningful (since it will return true for any empty string),
so why not just get rid of it, and |mNullStr| along with it?

Comment 8

[Blake Ross]

Comment on attachment 59362 [details] [diff] [review]
patch addressing dbaron's comments

sr=blake

Comment 9

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

So now it looks like the page load numbers are 20 seconds slower.  I'm really
not sure why that would happen -- I would think ToLowerCase ought to be faster
than nsString::ToLowerCase, but I could be wrong...

Comment 10

[Alec Flett]

not sure either, but new modern scrollbars also went in in that same build,
could also be affecting pageload time

Comment 11

[Alec Flett]

ah, I see. ToLowerCase() is in fact slower, because it calls nsCRT::ToLower()
for every character in the string.. nsStr::ChangeCase does everything inline.

*sigh*

patch to make ToLowerCase() faster coming up

Comment 12

[Alec Flett]

Attachment: fast table-based lower/uppercase

nsCRT::ToLower works one character at a time, and is table-based, which means
that every character is getting written back out to memory..

I think that given a reasonably fast processor the arithmetic and
range-checking will be faster than a memory lookup (which is what
nsCRT::ToLower does)

Comment 13

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

Comment on attachment 59438 [details] [diff] [review]
fast table-based lower/uppercase

>-      result = nsCRT::strdup(temp.mStr);
>+      result = nsCRT::strdup(temp.get());

In theory, ToNewCString should be faster here, right, since
it doesn't have to do the length calculation again?

>+              *cp = 'A' + (ch - 'a');

>+              *cp = 'a' + (ch - 'A');

My trust in compilers' ability to optimize is low enough
that I think you might be better off rewriting this as:

*cp = ch - ('a' - 'A')

and

*cp = ch + ('a' - 'A')

Comment 14

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

Comment on attachment 59438 [details] [diff] [review]
fast table-based lower/uppercase

r=dbaron

Comment 15

[Alec Flett]

I've made those changes in my local tree, thanks for the tips

Comment 16

[Alec Flett]

Attachment: updated patch reflecting dbaron's comments

if this is super-reviewed tonight, someone please check it in (I'm gone for the
day after posting this..:)) so we don't see slowdowns tomorrow.

Comment 17

[Johnny Stenback (:jst)]

Fix checked in, sr=me.

Comment 18

[Daniel Bratell]

The figures are still in the 1380 range from the 1360 they were at before the
checkin. Could it be something else in the patch that slows pageload down? 

Comment 19

[Alec Flett]

no, I _highly_ doubt anything else in my patch caused the problem.

(however, I'm leaving this bug open because this bug is supposed to be for other
things)

Comment 20

[Alec Flett]

ok, so I may have been hasty there.
in any case, the two places to look are:
1) the whole strLower.ToLower() vs. ToLowerCase(strLower) - there might be some
slight fixed-time overhead to ToLowerCase(strLower)? I'm not sure on that.
2) nsCStringKey() on an nsDependentCString vs. and nsCString
since nsCStringKey's destructor takes an nsAFlatString, I just can't imagine
there is much difference.

Comment 21

[Alec Flett]

s/destructor/constructor/

Comment 22

[Alec Flett]

*sigh* 0.9.8 was way to short. moving all my bugs to 0.9.9

Comment 23

[Alec Flett]

ok, current status on this: there are only 2 consumers left:
http://lxr.mozilla.org/seamonkey/search?string=nsStr%3A%3A

(look at the end, at the files that are not in string/)

as a part of another bug, I'm making all other nsStr:: members private/protected
so that nobody is ever tempted to use nsStr directly again.
This bug will be fixed when we get rid of these two consumers of nsStr::Delete2()

Comment 24

[Alec Flett]

Attachment: fix the last 2 consumers

yah! this is it. bye bye.
can I get r=dbaron, sr=jag?

Comment 25

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

Comment on attachment 66367 [details] [diff] [review]
fix the last 2 consumers

r=dbaron

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 66367 [details] [diff] [review]
fix the last 2 consumers

sr=jst

Comment 27

[Alec Flett]

thanks guys! fix has landed.