Last Comment Bug 710054 - Add {nsString,nsCString}::SizeOf{In,Ex}cludingThis
: Add {nsString,nsCString}::SizeOf{In,Ex}cludingThis
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: 688125 710594
  Show dependency treegraph
 
Reported: 2011-12-12 16:21 PST by Nicholas Nethercote [:njn]
Modified: 2012-02-06 00:53 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (2.12 KB, patch)
2012-01-23 22:07 PST, Nicholas Nethercote [:njn]
bzbarsky: review-
Details | Diff | Splinter Review
patch v2 (2.12 KB, patch)
2012-01-31 21:49 PST, Nicholas Nethercote [:njn]
bzbarsky: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-12-12 16:21:25 PST
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.
Comment 1 Boris Zbarsky [:bz] 2011-12-13 17:03:15 PST
How do we plan to handle F_SHARED strings?
Comment 2 Nicholas Nethercote [:njn] 2011-12-13 17:29:37 PST
> 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.
Comment 3 Boris Zbarsky [:bz] 2011-12-13 18:44:20 PST
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.
Comment 4 Nicholas Nethercote [:njn] 2012-01-05 21:08:08 PST
But 710594 needs nsCString::SizeOfExcludingThis as well.
Comment 5 Nicholas Nethercote [:njn] 2012-01-09 01:47:30 PST
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?
Comment 6 Boris Zbarsky [:bz] 2012-01-09 06:21:38 PST
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).
Comment 7 Nicholas Nethercote [:njn] 2012-01-23 22:07:41 PST
Created attachment 590995 [details] [diff] [review]
patch

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?
Comment 8 Boris Zbarsky [:bz] 2012-01-31 20:58:04 PST
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....
Comment 9 Nicholas Nethercote [:njn] 2012-01-31 21:49:02 PST
Created attachment 593316 [details] [diff] [review]
patch v2

> 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.
Comment 10 Boris Zbarsky [:bz] 2012-01-31 22:28:53 PST
Comment on attachment 593316 [details] [diff] [review]
patch v2

OK, fair.  r=me
Comment 11 Nicholas Nethercote [:njn] 2012-02-05 18:31:50 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc977bb3b1d9
Comment 12 Marco Bonardo [::mak] 2012-02-06 00:53:45 PST
https://hg.mozilla.org/mozilla-central/rev/cc977bb3b1d9

Note You need to log in before you can comment on or make changes to this bug.