If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsSharableString needs SetLength and SetCapacity

RESOLVED FIXED in mozilla0.9.7

Status

()

Core
String
P2
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla0.9.7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

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

Comment 1

16 years ago
s/I'm not completely sure now./I'm not completely sure whether they do now./
(Assignee)

Comment 2

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

Comment 3

16 years ago
Created attachment 55515 [details] [diff] [review]
working patch
(Assignee)

Comment 4

16 years ago
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?
(Assignee)

Comment 5

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

Comment 6

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

Comment 7

16 years ago
Also we need a double-on-fault like nsString.
(Assignee)

Comment 8

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

Comment 9

16 years ago
Created attachment 56227 [details] [diff] [review]
revised patch, but still doesn't handle NULL-ness
(Assignee)

Comment 10

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

Updated

16 years ago
Target Milestone: mozilla0.9.6 → mozilla0.9.7
(Assignee)

Updated

16 years ago
Attachment #55515 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Attachment #56227 - Attachment is obsolete: true
(Assignee)

Comment 11

16 years ago
Created attachment 56969 [details] [diff] [review]
patch that handles NULL-ness correctly (avoids null buffer handle internally)
(Assignee)

Comment 12

16 years ago
Created attachment 56970 [details] [diff] [review]
additional patch for testing of real patch, not for checkin

Comment 13

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

16 years ago
*** Bug 109972 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 15

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

(Assignee)

Comment 16

16 years ago
Created attachment 58266 [details] [diff] [review]
revised patch (a few off-by-one errors fixed, comments fixed)
Attachment #56969 - Attachment is obsolete: true
(Assignee)

Updated

16 years ago
Blocks: 74726
(Assignee)

Updated

16 years ago
Blocks: 100751

Comment 17

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

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

16 years ago
sr=scc

Comment 20

16 years ago
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+
(Assignee)

Comment 21

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

Comment 22

16 years ago
Fix checked in 2001-11-27 21:22 PDT.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Assignee)

Comment 23

16 years ago
Filed some of the issues mentioned in the first few comments as bug 114140.
You need to log in before you can comment on or make changes to this bug.