Closed Bug 234864 Opened 20 years ago Closed 20 years ago

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

Categories

(Core :: XPCOM, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.7beta

People

(Reporter: darin.moz, Assigned: darin.moz)

References

()

Details

(Keywords: perf, regression)

Attachments

(1 file)

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.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.7beta
the fact that we no longer share string buffers between JS and C++ could also
have something to do with this spike.
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.
actually, these results may be inaccurate... more later.
$ ./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.
Blocks: 231995
Attached patch v1 patchSplinter Review
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.
>+          if (!mAllocCount && !mFreeCount && !mAdoptCount)

this can just be |if (!mAllocCount && !mAdoptCount)| ... i've made this change
in my local tree.
Attachment #141777 - Flags: superreview?(dbaron)
Attachment #141777 - Flags: review?(dbaron)
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.
Attachment #141777 - Flags: superreview?(dbaron)
Attachment #141777 - Flags: superreview+
Attachment #141777 - Flags: review?(dbaron)
Attachment #141777 - Flags: review+
landing patch.  keeping bug open until we find out if this did the trick.
>   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.
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?
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
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: