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.
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.
Hm, 64 bytes also appears to be long enough. I'm starting to disbelieve my results here...
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!
Created attachment 599548 [details] [diff] [review] Patch v1
In retrospect, allocating two pages of stack space without checking that it's needed was pretty dumb.
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 on attachment 599548 [details] [diff] [review] Patch v1 [Approval Request Comment] Please see comment above.
(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.
I seriously doubt this has any snappy impact. The stack allocation is free, if we don't crash. :)
My fault. I was in the wrong tab when I applied the snappy tag.
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?)
8192 *bytes*. - nsAutoTArray<PRUint8, 8192, Alloc> temp; + nsAutoTArray<PRUint8, 64, Alloc> temp;
Oh, I missed that it's always PRUint8 there :)
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.
Fixed for Aurora 12: https://hg.mozilla.org/releases/mozilla-aurora/rev/8655ba93f381
Comment on attachment 599548 [details] [diff] [review] Patch v1 Requesting re-triage for mozilla-beta given bug 725945 comment 23.
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.
Fixed for Beta 11: https://hg.mozilla.org/releases/mozilla-beta/rev/787463417574
Is there anything QA can do to verify this fix?
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20) > Is there anything QA can do to verify this fix? No.
Tracking for the ESR given top ESR crasher bug 725945. Please land on that branch as soon as possible.
Hm, I put "a=akeybl" in the commit by muscle memory; sorry, Lukas! :)