Reduce nsTArray::SwapArrayElements's stack allocation from 8kb to something much smaller

RESOLVED FIXED in Firefox 11

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla13
Points:
---

Firefox Tracking Flags

(firefox11 fixed, firefox12 fixed, firefox13 fixed, firefox-esr1014+ fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 725945
(Assignee)

Comment 2

5 years ago
Hm, 64 bytes also appears to be long enough.  I'm starting to disbelieve my results here...
(Assignee)

Comment 3

5 years ago
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!
(Assignee)

Comment 4

5 years ago
Created attachment 599548 [details] [diff] [review]
Patch v1
Attachment #599548 - Flags: review?(roc)
(Assignee)

Comment 5

5 years ago
In retrospect, allocating two pages of stack space without checking that it's needed was pretty dumb.
Attachment #599548 - Flags: review?(roc) → review+
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Comment on attachment 599548 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Please see comment above.
Attachment #599548 - Flags: approval-mozilla-beta?
Attachment #599548 - Flags: approval-mozilla-aurora?

Comment 8

5 years ago
(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.
Whiteboard: [snappy]
(Assignee)

Comment 9

5 years ago
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.
Whiteboard: [snappy]
https://hg.mozilla.org/mozilla-central/rev/d346d9fe5134
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13

Comment 12

5 years ago
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?)
(Assignee)

Comment 13

5 years ago
8192 *bytes*.

-  nsAutoTArray<PRUint8, 8192, Alloc> temp;
+  nsAutoTArray<PRUint8, 64, Alloc> temp;

Comment 14

5 years ago
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.
Attachment #599548 - Flags: approval-mozilla-beta?
Attachment #599548 - Flags: approval-mozilla-beta-
Attachment #599548 - Flags: approval-mozilla-aurora?
Attachment #599548 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 16

5 years ago
Fixed for Aurora 12:

https://hg.mozilla.org/releases/mozilla-aurora/rev/8655ba93f381
status-firefox12: --- → fixed
status-firefox13: --- → fixed
(Assignee)

Comment 17

5 years ago
Comment on attachment 599548 [details] [diff] [review]
Patch v1

Requesting re-triage for mozilla-beta given bug 725945 comment 23.
Attachment #599548 - Flags: approval-mozilla-beta- → approval-mozilla-beta?
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.
Attachment #599548 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(Assignee)

Comment 19

5 years ago
Fixed for Beta 11: https://hg.mozilla.org/releases/mozilla-beta/rev/787463417574
status-firefox11: --- → fixed
Is there anything QA can do to verify this fix?
Whiteboard: [qa?]
(Assignee)

Comment 21

5 years ago
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #20)
> Is there anything QA can do to verify this fix?

No.
Whiteboard: [qa?] → [qa-]
Tracking for the ESR given top ESR crasher bug 725945. Please land on that branch as soon as possible.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 14+
(Assignee)

Updated

5 years ago
Attachment #599548 - Flags: approval-mozilla-esr10?
Attachment #599548 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/2255b5d15071
status-firefox-esr10: affected → fixed
(Assignee)

Comment 24

5 years ago
Hm, I put "a=akeybl" in the commit by muscle memory; sorry, Lukas!  :)
You need to log in before you can comment on or make changes to this bug.