104663 - nsSharableString needs SetLength and SetCapacity

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

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

First, the reason nsXPIDLString::Assign doesn't work is that its SetLength and
SetCapacity are the empty implementations on nsAString.

However, before we rush ahead to implement them, I think we should fix up that
part of the string API.  I propose that we replace the following methods:

nsAString::SetLength
nsAString::SetCapacity
nsAString::Truncate

with the following

nsAString::Truncate(PRUint32 aLength = 0)
nsAString::EnsureCapacity(PRUint32 aLength)
nsString::ReleaseStorage()

Right now, SetLength does 2 things:
    * if the length is shorter than the current length, it truncates the string
      (the same as truncate)
    * if the length is longer than the current length, I *think* it expands the
      capacity if necessary, and then extends the length so that some characters 
      in the string are garbage
  I think the latter of these two should not exist, and the former should become 
  (and already is) |Truncate|.

Right now, SetCapacity does two things:
    * Expands the allocated capacity of the string if the given length is below
      the current capacity.
    * |SetCapacity(0)| frees storage associated with the string.  This is only
      used in two places (CNavDTD.cpp and nsGtkIMEHelper.cpp), and I propose we
      put ReleaseStorage on nsString until we decide what to do with these two
      callers.
  If we remove the second meaning of SetCapacity I think it's appropriate to
  rename it to |EnsureCapacity|.

Finally, I think the sizes given to all these functions should *never* include a
terminating null -- they should be equivalent to a length -- i.e.,
SetCapacity(32) on a flat string type would require allocating
|33 * sizeof(char_type)| bytes.  I'm not completely sure now.

Comment 1

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

s/I'm not completely sure now./I'm not completely sure whether they do now./

Comment 2

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

I've realized we do need SetLength for now, since that's the way everything
writes into the string.  It's ugly, but that's the way things currently work. 
So I think the best short-term solution is just to implement these methods on
nsSharableString.

Comment 3

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

Attachment: working patch

Comment 4

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

The above patch works.  Mozilla runs.  I ran some summary leak and bloat stats
-- this patch doesn't leak, but it increases total allocation a good bit, mainly
since I didn't convert the XUL attributes to use nsSharableString, which would
be rather difficult.  (The real solution here may be nsStackString, our
replacement for nsAutoString that works with sharing, which we don't have yet. 
We could probably also make atoms participate in the string hierarchy.)

I think we need to move away from allowing NULL-ness, not towards it.  Right now
sharable strings' |get()| can return null.  We need the shared-empty-buffer
hack, I think, but attacking nsXPIDLString is a good bit of work.

The main thing I did here that might be controversial is removing
nsFlexBufferHandle and adding a word to nsSharedBufferHandle.  The additional
word on nsSharedBufferHandle was needed to make SetCapacity at all useful, which
I think it probably needs to be (although once we convert nsString to a typedef
to nsSharableString we could do performance testing to see if this is really
true).  I don't see how nsFlexBufferHandle is useful -- it wouldn't be all that
useful for a Cut since the cut would still require a copy if the buffer was
shared, and I'd think a better solution for that problem would be a sharing
substring class that owns references to the buffer (or buffers) that it is a
substring of.  Was there some other need for having StorageEnd != DataEnd in the
nsFlexBufferHandle?

Comment 5

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

(FWIW, I'm only assuming that it's the XUL attributes issue that's causing total
allocations to increase.  Also, I still need to verify in a debugger that
sharing is actually happening...)

Comment 6

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

Oops, I forgot to actually enforce the:

+        // Since sharing should be transparent to the caller, we should
+        // therefore *unconditionally* truncate the current length of
+        // the string to the requested capacity.

Comment 7

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

Also we need a double-on-fault like nsString.

Comment 8

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

do_AssignFromReadable could fail if |aReadable| is dependent (e.g., a dependent
concatentation) on |this|.

Comment 9

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

Attachment: revised patch, but still doesn't handle NULL-ness

Comment 10

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

NULL-ness problems include things like:

#1  0x402071c8 in nsDebug::Assertion(char const*, char const*, char const*, int)
(aStr=0x402825e5 "trouble: no buffer!", aExpr=0x402825de "buffer", 
    aFile=0x402825a0
"/builds/string/mozilla/string/src/nsASingleFragmentString.cpp", aLine=36) at
/builds/string/mozilla/xpcom/base/nsDebug.cpp:290
#2  0x4022acb2 in
nsASingleFragmentString::GetReadableFragment(nsReadableFragment<unsigned
short>&, nsFragmentRequest, unsigned) const (this=0xbfffc490, 
    aFragment=@0xbfffc240, aRequest=kFirstFragment, aOffset=0)
    at /builds/string/mozilla/string/src/nsASingleFragmentString.cpp:36
#3  0x40248a38 in nsAString::BeginReading(nsReadingIterator<unsigned short>&)
const (this=0xbfffc490, aResult=@0xbfffc240)
    at ../../dist/include/string/nsAString.h:619
#4  0x4022b13a in Compare(nsAString const&, nsAString const&, nsStringComparator
const&) (lhs=@0xbfffc490, rhs=@0x8254fec, aComparator=@0xbfffc470)
    at /builds/string/mozilla/string/src/nsAString.cpp:56
#5  0x41a0e05b in SelectorMatches(SelectorMatchesData&, nsCSSSelector*, int,
signed char) (data=@0xbfffc5a0, aSelector=0x8254fa4, aTestState=1, 
    aNegationIndex=0 '\000')
    at /builds/string/mozilla/content/html/style/src/nsCSSStyleSheet.cpp:3684

I think I'm going to try to change nsSharableString and nsXPIDLString to always
have a real buffer handle, but make nsXPIDLString do some hacky things to return
NULL so the API is the same, for now (but also implement IsVoid / SetIsVoid).

Comment 11

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

Attachment: patch that handles NULL-ness correctly (avoids null buffer handle internally)

Comment 12

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

Attachment: additional patch for testing of real patch, not for checkin

Comment 13

[jag (Peter Annema)]

Comment on attachment 56970 [details] [diff] [review]
additional patch for testing of real patch, not for checkin

You might want to take a look at this code and see if it still works with your
change:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsScriptLoader.cpp#699

>Index: mozilla/js/src/xpconnect/src/xpcprivate.h
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/xpconnect/src/xpcprivate.h,v
>retrieving revision 1.109
>diff -u -d -r1.109 xpcprivate.h
>--- xpcprivate.h    2001/11/07 04:12:06    1.109
>+++ xpcprivate.h    2001/11/07 23:15:58
>@@ -2301,6 +2301,7 @@
> NS_REINTERPRET_CAST(PRUnichar *,
> JS_GetStringChars(str) +
> JS_GetStringLength(str)),
>+               JS_GetStringLength(str) + 1,

I think we should keep Capacity (StorageLength) to be the number of
characters we want to store for the string, not counting the zero
terminator.

>Index: mozilla/string/public/nsXPIDLString.h
>===================================================================
>RCS file: /cvsroot/mozilla/string/public/nsXPIDLString.h,v
>retrieving revision 1.5
>diff -u -d -r1.5 nsXPIDLString.h
>--- nsXPIDLString.h    2001/10/28 08:24:54    1.5
>+++ nsXPIDLString.h    2001/11/07 23:16:20
>@@ -32,9 +32,66 @@

...

>+   * Like nsCOMPtr, nsXPIDLString uses some syntactic sugar to make it
>+   * painfully clear exactly what the code expects. You need to wrap an
>+   * nsXPIDLString object with `getter_Copies()'
>+   * before passing it to a getter: these tell the
>+   * nsXPIDLString how ownership is being handled.

Wrapping...

>+   *
>+   * In the case of `getter_Copies()', the callee is allocating a copy
>+   * (which is usually the case). In the case where the
>+   * callee is returning a const reference to `the real deal' (this can
>+   * be done using the [shared] attribute in XPIDL) you can just use
>+   * a |const char*|.

const PRUnichar* / const char*

>Index: mozilla/string/src/nsSharableString.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/string/src/nsSharableString.cpp,v
>retrieving revision 1.5
>diff -u -d -r1.5 nsSharableString.cpp
>--- nsSharableString.cpp    2001/10/13 15:01:21    1.5
>+++ nsSharableString.cpp    2001/11/07 23:16:20
>@@ -25,13 +25,111 @@
>+void
>+nsSharableString::SetLength( size_type aNewLength )
> {
>+    if ( aNewLength > mBuffer->DataLength() )
>+      {
>+        SetCapacity(aNewLength);
>+        mBuffer->DataEnd( mBuffer->DataStart() + aNewLength );
>+      }
>+    else
>+      {
>+        mBuffer->DataEnd( mBuffer->DataStart() + aNewLength );
>+        *mBuffer->DataEnd() = char_type(0); // is this needed?

I think so.

Remind me to look at this bit below again:

>Index: mozilla/string/src/nsXPIDLString.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/string/src/nsXPIDLString.cpp,v
>retrieving revision 1.4
>diff -u -d -r1.4 nsXPIDLString.cpp
>--- nsXPIDLString.cpp    2001/10/13 15:01:21    1.4
>+++ nsXPIDLString.cpp    2001/11/07 23:16:20
>@@ -100,15 +108,21 @@
> const nsXPIDLString::shared_buffer_handle_type*
> nsXPIDLString::GetSharedBufferHandle() const
> {
>-    const nsImportedStringHandle<char_type>* answer = NS_STATIC_CAST(const nsImportedStringHandle<char_type>*, mBuffer.get());
>-    
>-    if ( answer && !answer->DataEnd() && answer->StorageStart() )
>-      answer->RecalculateBoundaries();
>+    self_type* mutable_this = NS_CONST_CAST(self_type*, this);
>+    if ( !mBuffer->DataStart() )
>+      // XXXldb This isn't any good.  What if we just called
>+      // PrepareForUseAsOutParam and it hasn't been filled in yet?
>+      mutable_this->mBuffer = GetSharedEmptyBufferHandle();
>+    else if ( !mBuffer->DataEnd() )
>+      // Our handle may not be an nsImportedStringHandle.  However, if it
>+      // is not, this cast will still be safe since no other handle will
>+      // be in this state.
>+      NS_STATIC_CAST(const nsImportedStringHandle<char_type>*, mBuffer.get())->RecalculateBoundaries();
>
> #if DEBUG_STRING_STATS
> ++sShareCount;
> #endif
>-    return answer;
>+    return mBuffer.get();
> }

Comment 14

[Darin Fisher]

*** Bug 109972 has been marked as a duplicate of this bug. ***

Comment 15

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

> I think we should keep Capacity (StorageLength) to be the number of
> characters we want to store for the string, not counting the zero
> terminator.

My inclination is to disagree.  I think the storage length should always include
room for the terminating null -- it's a description of the actual storage that
has been allocated.  On the other hand, for a string class that requires
null-termination, SetCapacity should ensure that there is room for the
terminating null.  Not all implementations of nsAString need require null
termination.

Comment 16

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

Attachment: revised patch (a few off-by-one errors fixed, comments fixed)

Comment 17

[Scott Collins]

I'm reviewing the patch now ... but it's a big patch and deserves careful
attention, so this will take me most of the day, I think.  Sorry for the delay
in getting to this.

Comment 18

[Scott Collins]

I've been over this patch (still looking, but I've got a pretty good grasp, I
think).  Some things surprised me at first, but then I remembered that we had
talked about them.  Nothing bad has jumped out yet.  More anon.

Comment 19

[Scott Collins]

sr=scc

Comment 20

[jag (Peter Annema)]

Comment on attachment 58266 [details] [diff] [review]
revised patch (a few off-by-one errors fixed, comments fixed)

r=jag

Comment 21

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

I just noticed there were some changes in nsAString.h that I didn't put in both
classes (mostly comments, but also removal of the length check in Truncate()). 
I'll attach an additional patch later...

Comment 22

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

Fix checked in 2001-11-27 21:22 PDT.

Comment 23

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

Filed some of the issues mentioned in the first few comments as bug 114140.