Closed Bug 1301025 Opened 3 years ago Closed 3 years ago

Change string allocation to be similar to nsTArray allocation

Categories

(Core :: String, defect, P3)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: smaug, Assigned: baku)

References

Details

Attachments

(1 file)

Priority: -- → P3
Attached patch string.patchSplinter Review
Assignee: nobody → amarchesini
Attachment #8789371 - Flags: review?(nfroyd)
Attachment #8789371 - Flags: review?(bugs)
Comment on attachment 8789371 [details] [diff] [review]
string.patch

Review of attachment 8789371 [details] [diff] [review]:
-----------------------------------------------------------------

r=me assuming we can come to some reasonable consensus about the particular constants involved here.  I'd be interested to hear thoughts on what we think "large" strings would be in a web context.

::: xpcom/string/nsTSubstring.cpp
@@ +80,5 @@
> +    // We increase our capacity so that the allocated buffer grows
> +    // exponentially, which gives us amortized O(1) appending. Below the
> +    // threshold, we use powers-of-two. Above the threshold, we grow by at
> +    // least 1.125, rounding up to the nearest MiB.
> +    const size_type slowGrowthThreshold = 8 * 1024 * 1024;

I don't have any concerns with the code; I'm just wondering if we should tweak the values a little bit, as ns*String usage is a bit different from nsTArray usage.  At the very least, the element size is anywhere from an eighth (nsCString, 64-bit platform) to half (nsString, 32-bit platform) of what I'd expect a "typical" nsTArray element to be (pointer-sized).

Proposed numbers: slowGrowthThreshold at 1MB and rounding up to the nearest quarter-megabyte?
Attachment #8789371 - Flags: review?(nfroyd) → review+
Based on http://searchfox.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#79-82 we do want to increase at least in size of 1MB at a time.
I can test the string lengths for strings loading the most common websites if this can be important to test.
Otherwise I think we can stay with: slowGrowthThreshold at 1MB without changing the rounding up.
Comment on attachment 8789371 [details] [diff] [review]
string.patch

># HG changeset patch
># User Andrea Marchesini <amarchesini@mozilla.com>
># Parent  e8100c976750e9f5ddc4d18aa04b87a3efb736bc
>
>diff --git a/xpcom/string/nsTSubstring.cpp b/xpcom/string/nsTSubstring.cpp
>--- a/xpcom/string/nsTSubstring.cpp
>+++ b/xpcom/string/nsTSubstring.cpp
>@@ -69,24 +69,40 @@ nsTSubstring_CharT::MutatePrep(size_type
>   // need to allocate a new buffer. We cannot use the existing buffer even
>   // though it might be large enough.
> 
>   if (curCapacity != 0) {
>     if (aCapacity <= curCapacity) {
>       mFlags &= ~F_VOIDED;  // mutation clears voided flag
>       return true;
>     }
>+  }
> 
>-    // Use doubling algorithm when forced to increase available capacity.
>-    size_type temp = curCapacity;
>-    while (temp < aCapacity) {
>-      temp <<= 1;
>+  if (curCapacity < aCapacity) {
>+    // We increase our capacity so that the allocated buffer grows
>+    // exponentially, which gives us amortized O(1) appending. Below the
>+    // threshold, we use powers-of-two. Above the threshold, we grow by at
>+    // least 1.125, rounding up to the nearest MiB.
>+    const size_type slowGrowthThreshold = 8 * 1024 * 1024;
>+
>+    size_type temp;
>+    if (aCapacity >= slowGrowthThreshold) {
>+      size_type minNewCapacity = curCapacity + (curCapacity >> 3); // multiply by 1.125
>+      temp = XPCOM_MIN(aCapacity, minNewCapacity);
You want XPCOM_MAX here. Otherwise you may not allocate enough, right?
That is what nsTArray is doing too.

with that, r+
Attachment #8789371 - Flags: review?(bugs) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf84879a461
Change string allocation to be similar to nsTArray allocation, r=smaug, r=froydnj
https://hg.mozilla.org/mozilla-central/rev/ccf84879a461
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Wouldn't line 116 (Post patch) cause the size given to the "realloc" function to be unaligned to the next power of 2? (After being rounded in lines 87-97) 

After calculating the nearest power of two, it allocates that amount + 1, wouldn't it cause the "realloc" function to use a larger allocation than intended?
Flags: needinfo?(bugs)
Flags: needinfo?(bugs)
Hmm, that didn't work. I was trying to forward the needinfo request to the patch author.
But indeed, that seems to be the case. That line was unfortunately not visible in the patch.
http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/xpcom/string/nsTSubstring.cpp#116
Flags: needinfo?(amarchesini)
Depends on: 1324808
Though , that issue has been there forever, in some form.
Flags: needinfo?(amarchesini)
You need to log in before you can comment on or make changes to this bug.