Closed Bug 710054 Opened 12 years ago Closed 12 years ago

Add {nsString,nsCString}::SizeOf{In,Ex}cludingThis

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 1 obsolete file)

We need to add nsString::SizeOf{In,Ex}cludingThis.  Bug 688125's patch will need to be fixed up once the new methods are added.
Whiteboard: [MemShrink] → [MemShrink:P2]
How do we plan to handle F_SHARED strings?
> How do we plan to handle F_SHARED strings?

Good question!  We'll just have to not count them, and likewise for F_FIXED strings IIUC.

There's precedent for this -- we don't count the chars for external JSStrings because we don't know who owns them.
The thing is, F_SHARED may well be the common case.

Maybe we should do something like this:

  if ((mFlags & F_SHARED) && nsStringBuffer::FromData(mData)->IsReadonly())
    return 0;

?  That would still count F_SHARED strings that have a writable stringbuffer (which means the buffer refcount is 1, so we really do own them at the moment).  The good news is that this would account for things better; the bad news is that it would make memory suddenly stop being accounted for when someone else takes a reference...  But there may be no way to win against that with any sort of solution that accounts for F_SHARED strings in any way.
But 710594 needs nsCString::SizeOfExcludingThis as well.
Summary: Add nsString::SizeOf{In,Ex}cludingThis → Add {nsString,nsCString}::SizeOf{In,Ex}cludingThis
Blocks: 710594
I just tried to implement these functions but I was bamboozled by the inheritance hierarchy:  nsString, nsStringContainer, and the very mysterious nsStringContainer_base.  Not to mention all those NS_String* functions.  I can't even work out what nsString's data members are.  

bz, can you please give me a hint?
nsStringContainer and nsStringContainer_base are the "external" string API.  They're basically not used by our own code, but only by binary extensions.

nsStringContainer_base is basically just a struct that has the same data members as the actual concrete string implementation we use, so that one can reinterpret_cast between the two.

For purposes of Gecko code, the relevant file is xpcom/string/public/nsTSubstring.h.  This defines the nsTSubstring_CharT class, which corresponds to nsAString and nsACString (the file is #included twice with different types as char_type; think of it as a poor-man's template dating back to when we had to compile on compilers with broken template support).  xpcom/string/public/nsAString.h has the #include magic if you want to read through it.

In any case, nsTSubstring_CharT has these data members:

  605       char_type*  mData;
  606       size_type   mLength;
  607       PRUint32    mFlags;

All other string classes we use internaly inherit from nsTSubstring_CharT.  Some have additional data members (almost certainly just for inline buffers).
Blocks: 688125
Attached patch patch (obsolete) — Splinter Review
I used names like the ones in the patch 2 in bug 671299.

I didn't do the IsReadonly() part of the shared test like comment 3 suggested because nsStringBuffer isn't visible in nsTSubstring.h.

I fixed some indenting just above the inserted code.  (The indenting style in that file is awful.)

There are no actual users of these functions, which makes me nervous.  Bug 688125 and bug 710594 are depending on this, maybe we'll just let them find out if there's a problem?
Attachment #590995 - Flags: review?(bzbarsky)
Comment on attachment 590995 [details] [diff] [review]
patch

Both of the IncludingThis functions should be calling through to the _ExcludingThis_ functions.  Instead, they're calling themselves, so if someone tries to use them they'll just blow out the stack.

What are the use cases for the MustBeUnshared versions?

And I'm sorry for the lag here....
Attachment #590995 - Flags: review?(bzbarsky) → review-
Attached patch patch v2Splinter Review
> Both of the IncludingThis functions should be calling through to the
> _ExcludingThis_ functions.  Instead, they're calling themselves, so if
> someone tries to use them they'll just blow out the stack.

LOL, I've fixed that in the new patch.


> What are the use cases for the MustBeUnshared versions?

It's for strings where you know they're unshared and you want this to be checked.  It was useful in nsStringBuffer (see https://hg.mozilla.org/integration/mozilla-inbound/rev/81de95739be6, you r+'d the patch :) so I thought it might be useful here too.
Attachment #590995 - Attachment is obsolete: true
Attachment #593316 - Flags: review?(bzbarsky)
Comment on attachment 593316 [details] [diff] [review]
patch v2

OK, fair.  r=me
Attachment #593316 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/cc977bb3b1d9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.