Last Comment Bug 729453 - Reduce nsTArray::SwapArrayElements's stack allocation from 8kb to something much smaller
: Reduce nsTArray::SwapArrayElements's stack allocation from 8kb to something m...
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on:
Blocks: 725945
  Show dependency treegraph
 
Reported: 2012-02-22 02:12 PST by Justin Lebar (not reading bugmail)
Modified: 2012-07-06 06:17 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed
14+
fixed


Attachments
Patch v1 (1.41 KB, patch)
2012-02-22 03:29 PST, Justin Lebar (not reading bugmail)
roc: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Justin Lebar (not reading bugmail) 2012-02-22 02:12:59 PST
This may be causing or contributing to the crashes seen in bug 725945.

8kb seems pretty excessive, in retrospect.  I bet 256 bytes would be just fine.
Comment 1 Justin Lebar (not reading bugmail) 2012-02-22 02:37:38 PST
I did a quick test with 256 bytes: Visited gmail, slashdot, techcrunch.  We hit the autotarray --> tarray swap path ~500,000 times, and every single time, 256 bytes was large enough.
Comment 2 Justin Lebar (not reading bugmail) 2012-02-22 03:01:15 PST
Hm, 64 bytes also appears to be long enough.  I'm starting to disbelieve my results here...
Comment 3 Justin Lebar (not reading bugmail) 2012-02-22 03:22:45 PST
Huh, even 8 bytes is sufficient for all callers.  I think this is because the vast majority of swap calls have at least one empty array, and we allocate only if the smallest array's length is larger than the capacity of the temporary autotarray.

In any case, 64 bytes seems like a reasonable place to go for now!
Comment 4 Justin Lebar (not reading bugmail) 2012-02-22 03:29:13 PST
Created attachment 599548 [details] [diff] [review]
Patch v1
Comment 5 Justin Lebar (not reading bugmail) 2012-02-22 05:54:22 PST
In retrospect, allocating two pages of stack space without checking that it's needed was pretty dumb.
Comment 6 Justin Lebar (not reading bugmail) 2012-02-23 09:46:09 PST
This change should be quite safe.  It's reducing the stack space used by a temporary variable.  If we need to use more space than is available on the stack, we fall back to using malloc().  In my tests, this fallback is never hit, with or without the patch here.

So I'm quite comfortable taking this change on aurora.

Beta may be a higher bar -- we have no evidence that this change actually *fixes* anything, and I'm concerned that this patch could, in some case I didn't test, cause us to malloc() more often in SwapArrayElements, which could change behavior.

I think it's unlikely that this will be a problem, but I'm hesitant to churn beta without evidence that we're fixing a problem.
Comment 7 Justin Lebar (not reading bugmail) 2012-02-23 09:46:37 PST
Comment on attachment 599548 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Please see comment above.
Comment 8 Scoobidiver (away) 2012-02-23 09:55:29 PST
(In reply to Justin Lebar [:jlebar] from comment #6)
> I think it's unlikely that this will be a problem, but I'm hesitant to churn
> beta without evidence that we're fixing a problem.
We can wait 4 days (until Feb 28) to observe effects on Aurora before eventually landing it in 11.0b5.
Comment 9 Justin Lebar (not reading bugmail) 2012-02-23 14:43:59 PST
I seriously doubt this has any snappy impact.  The stack allocation is free, if we don't crash.  :)
Comment 10 Lawrence Mandel [:lmandel] (use needinfo) 2012-02-23 14:50:42 PST
My fault. I was in the wrong tab when I applied the snappy tag.
Comment 11 Richard Newman [:rnewman] 2012-02-23 18:55:49 PST
https://hg.mozilla.org/mozilla-central/rev/d346d9fe5134
Comment 12 Jesse Ruderman 2012-02-26 08:23:14 PST
This is a number of entries, not a number of bytes.  Allocating 8192 *entries* on the stack is crazy!  (Does this help explain bug 725945 and change the beta calculus?)
Comment 13 Justin Lebar (not reading bugmail) 2012-02-26 18:57:56 PST
8192 *bytes*.

-  nsAutoTArray<PRUint8, 8192, Alloc> temp;
+  nsAutoTArray<PRUint8, 64, Alloc> temp;
Comment 14 Jesse Ruderman 2012-02-27 01:20:26 PST
Oh, I missed that it's always PRUint8 there :)
Comment 15 Alex Keybl [:akeybl] 2012-02-27 16:23:32 PST
Comment on attachment 599548 [details] [diff] [review]
Patch v1

[Triage Comment]
Approving for Aurora 12 to speed up the investigation, but denying for Beta 11 since we're past the point of landing speculative fixes for release.
Comment 16 Justin Lebar (not reading bugmail) 2012-02-28 08:55:16 PST
Fixed for Aurora 12:

https://hg.mozilla.org/releases/mozilla-aurora/rev/8655ba93f381
Comment 17 Justin Lebar (not reading bugmail) 2012-02-28 09:02:20 PST
Comment on attachment 599548 [details] [diff] [review]
Patch v1

Requesting re-triage for mozilla-beta given bug 725945 comment 23.
Comment 18 Alex Keybl [:akeybl] 2012-02-28 14:13:50 PST
Comment on attachment 599548 [details] [diff] [review]
Patch v1

[Triage Comment]
smooney let us know that that she talked with Justin and we think that any regressions from this will be in crash data, which will give us an opportunity to still back out the change before release. Approving for Beta 11.
Comment 19 Justin Lebar (not reading bugmail) 2012-02-28 15:24:30 PST
Fixed for Beta 11: https://hg.mozilla.org/releases/mozilla-beta/rev/787463417574
Comment 20 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-03-05 16:41:28 PST
Is there anything QA can do to verify this fix?
Comment 21 Justin Lebar (not reading bugmail) 2012-03-05 16:46:50 PST
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> Is there anything QA can do to verify this fix?

No.
Comment 22 Alex Keybl [:akeybl] 2012-07-05 16:19:16 PDT
Tracking for the ESR given top ESR crasher bug 725945. Please land on that branch as soon as possible.
Comment 23 Justin Lebar (not reading bugmail) 2012-07-06 06:17:16 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/2255b5d15071
Comment 24 Justin Lebar (not reading bugmail) 2012-07-06 06:17:45 PDT
Hm, I put "a=akeybl" in the commit by muscle memory; sorry, Lukas!  :)

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