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)
Tracking
()
RESOLVED
FIXED
mozilla1.7beta
People
(Reporter: darin.moz, Assigned: darin.moz)
References
()
Details
(Keywords: perf, regression)
Attachments
(1 file)
39.74 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•20 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.7beta
Assignee | ||
Comment 1•20 years ago
|
||
the fact that we no longer share string buffers between JS and C++ could also have something to do with this spike.
Assignee | ||
Comment 2•20 years ago
|
||
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.
Assignee | ||
Comment 3•20 years ago
|
||
actually, these results may be inaccurate... more later.
Assignee | ||
Comment 4•20 years ago
|
||
$ ./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.
Assignee | ||
Comment 5•20 years ago
|
||
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.
Assignee | ||
Comment 6•20 years ago
|
||
>+ if (!mAllocCount && !mFreeCount && !mAdoptCount)
this can just be |if (!mAllocCount && !mAdoptCount)| ... i've made this change
in my local tree.
Assignee | ||
Updated•20 years ago
|
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+
Assignee | ||
Comment 8•20 years ago
|
||
landing patch. keeping bug open until we find out if this did the trick.
Assignee | ||
Comment 9•20 years ago
|
||
> 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.
Assignee | ||
Comment 10•20 years ago
|
||
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
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•