Closed Bug 104663 Opened 23 years ago Closed 23 years ago

nsSharableString needs SetLength and SetCapacity

Categories

(Core :: XPCOM, defect, P2)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.7

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(2 files, 3 obsolete files)

additional patch for testing of real patch, not for checkin
3.14 KB, patch
Details | Diff | Splinter Review
revised patch (a few off-by-one errors fixed, comments fixed)
64.71 KB, patch
jag+mozilla
: review+
jag+mozilla
: superreview+
Details | Diff | Splinter Review 
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.
s/I'm not completely sure now./I'm not completely sure whether they do now./
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.
Status: NEW → ASSIGNED
Priority: -- → P2
Summary: SetLength and SetCapacity → nsSharableString needs SetLength and SetCapacity
Target Milestone: --- → mozilla0.9.6
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?
(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...)
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.
Also we need a double-on-fault like nsString.
do_AssignFromReadable could fail if |aReadable| is dependent (e.g., a dependent
concatentation) on |this|.
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).
Target Milestone: mozilla0.9.6 → mozilla0.9.7
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();
> }
*** Bug 109972 has been marked as a duplicate of this bug. ***
> 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.


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.
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.
sr=scc
Comment on attachment 58266 [details] [diff] [review]
revised patch (a few off-by-one errors fixed, comments fixed)

r=jag
Attachment #58266 - Flags: superreview+
Attachment #58266 - Flags: review+
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...
Fix checked in 2001-11-27 21:22 PDT.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Filed some of the issues mentioned in the first few comments as bug 114140.
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: