234864 - string branch landing resulted in large spike in heap allocations (brad:A metric)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Darin Fisher]

string branch landing resulted in large spike in heap allocations (brad:A metric).

228K -> 319K (~40% increase!!)

dbaron's point #4 from bug 232927 may very well explain this increase, but some
trace-malloc investigation is probably warranted here to uncover the cause of
this large increase.

Comment 1

[Darin Fisher]

the fact that we no longer share string buffers between JS and C++ could also
have something to do with this spike.

Comment 2

[Darin Fisher]

I added a string stat to capture cases where we don't use a nsAutoString's stack
buffer even though we could.  Running TestStandardURL shows the following:

$ ./TestStandardURL
http://www.mozilla.org/projects/netlib/http/http-debugging.html 10000

nsStringStats
 => mAllocCount: 30470
 => mReallocCount: 286
 => mFreeCount: 30452
 => mShareCount: 10489
 => mAdoptCount: 6
 => mAdoptFreeCount: 6
 => mSkippedFixedBuf: 50485

notice that that is nearly 5x as many missed stack allocations as string buffers
that were shared.  this suggests that increased sharing isn't really the result
of not using the stack buffers.  note: we share 10000 times corresponding to
10000 GetSpec calls, and the other 489 shared buffers corresponds to XPCOM
initialization stuff.  when my build finishes, i'll see what the numbers look
like for normal browsing.  it shouldn't take much work to create a patch if this
proves to be the problem.

Comment 3

[Darin Fisher]

actually, these results may be inaccurate... more later.

Comment 4

[Darin Fisher]

$ ./mozilla http://www.cnn.com/

loaded with trunk, measuring cases where the nsAutoString's buffer is not used
but could have been used.

nsStringStats
 => mAllocCount: 57833
 => mReallocCount: 40801
 => mFreeCount: 57832
 => mShareCount: 26248
 => mAdoptCount: 2380
 => mAdoptFreeCount: 2380
 => mSkippedFixedBuf: 35399

w/ patch to use nsAutoString's buffer whenever possible:

StringStats
 => mAllocCount: 22200
 => mReallocCount: 5845
 => mFreeCount: 22199
 => mShareCount: 19434
 => mAdoptCount: 2397
 => mAdoptFreeCount: 2397
 => mSkippedFixedBuf: 0

based on these figures i think the patch is likely a very good idea.

Comment 5

[Darin Fisher]

Attachment: v1 patch

Here's a cleaned up version of the patch I was testing.  It introduces
nsTFixedString which inherits from nsTString.  nsTAutoString now inherits from
nsTFixedString.  nsTSubstring casts itself to nsTFixedString if it has the
F_HAS_FIXEDBUF flag set.  This enables nsTSubstring to locate the fixed buffer
even when mData points to some other buffer.  I've also eliminated
CBufDescriptor in this patch since nsTFixedString subsumes its functionality. 
There were only a few users of CBufDescriptor.	This is a pretty
straightforward change.

Comment 6

[Darin Fisher]

>+          if (!mAllocCount && !mFreeCount && !mAdoptCount)

this can just be |if (!mAllocCount && !mAdoptCount)| ... i've made this change
in my local tree.

Comment 7

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

Comment on attachment 141777 [details] [diff] [review]
v1 patch

r+sr=dbaron, except I'd prefer replacing the bit twiddling with:

  PRUint16 mFlags;
  PRUint16 mClassFlags;

and using F_CLASS_FIXED = 1<<0
or something like that.

Comment 8

[Darin Fisher]

landing patch.  keeping bug open until we find out if this did the trick.

Comment 9

[Darin Fisher]

>   PRUint16 mFlags;
>   PRUint16 mClassFlags;
> 
> and using F_CLASS_FIXED = 1<<0
> or something like that.

dbaron and i discussed this.  we agreed that splitting flags into two 16-bit
integers would only complicate string constructors.  it's probably worth the bit
twiddling when we need to change data flags in order to benefit from minimal
code at ctor callsites.

also, i switched to F_CLASS_FIXED per dbaron's suggestion.

Comment 10

[Darin Fisher]

Well, the patch definitely worked.  brad:A dropped back down to 230K, which is
very close to the pre- string landing numbers.

the patch introduced a significant increase in codesize however:

[brad]
 Zdiff: +44360
mZdiff: +24564

[luna]
 Zdiff: +48288
mZdiff: +25824

[beast]
 Zdiff: +40066
mZdiff: +20524

i think this codesize increase is tolerable given the much greater gains in
codesize from the overall string branch landing.  the cause of this increase is
likely due to the additional member variable initializer in every nsTAutoString.
 we could probably invent a class flag to get around that problem, but i'm not
sure that additional complexity is worth it.  thoughts?