Closed Bug 1166823 Opened 4 years ago Closed 4 years ago

"RangeError: arguments array passed to Function.prototype.apply is too large" when recording octane

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(firefox40 fixed, firefox41 fixed)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox40 --- fixed
firefox41 --- fixed

People

(Reporter: fitzgen, Assigned: jsantell)

References

Details

Attachments

(1 file, 1 obsolete file)

After fixing bug 1160811 locally, I get this error a bunch in the console while recording octane:

RangeError: arguments array passed to Function.prototype.apply is too large: RecordingModel.prototype._addTimelineData@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/recording-model.js:377:9
PerformanceActorsConnection.prototype._onTimelineData/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/front.js:292:35
PerformanceActorsConnection.prototype._onTimelineData@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/front.js:292:5
EventEmitter_emit@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/event-emitter.js:147:11
MemoryFrontFacade.prototype._pullAllocationSites<@resource://gre/modules/commonjs/toolkit/loader.js -> resource:///modules/devtools/performance/actors.js:369:1
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:867:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:746:7
this.PromiseWalker.scheduleWalkerLoop/<@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:37
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:688:5
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:709:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:671:7
Front<.onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/server/protocol.js:1217:7
DebuggerClient.prototype.onPacket@resource://gre/modules/devtools/dbg-client.jsm:953:7
LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/transport/transport.js:561:11
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://gre/modules/devtools/DevToolsUtils.js:82:14
Works:
test.apply(null, new Array(500000).join("x").split("x"))

Throws:
test.apply(null, new Array(500001).join("x").split("x"))

Pushing boundaries over here.
And recording-model.js:377:9 looks like this:

> Array.prototype.push.apply(this._allocations.frames, frames);

I think we need to just do a for loop + individual push. I think there shouldn't be any perf difference after ion gets its hands on it.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Attached patch 1166823-array-concat.patch (obsolete) — Splinter Review
Attachment #8608284 - Flags: review?(nfitzgerald)
Comment on attachment 8608284 [details] [diff] [review]
1166823-array-concat.patch

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

r=me with below addressed

::: browser/devtools/performance/modules/recording-model.js
@@ +384,5 @@
>    toString: () => "[object RecordingModel]"
>  };
>  
> +/**
> + * Concatenates elements of array2 in array1. Marker data will come in small chunks

We already have Array.prototype.concat (which we definitely don't want to use because it creates a new copy) so this wording is a little misleading because of that. How about "Push all elements of src (nee array2) to dest (nee array1)" instead of using the word "concatenates".

@@ +386,5 @@
>  
> +/**
> + * Concatenates elements of array2 in array1. Marker data will come in small chunks
> + * and add up over time, whereas allocation arrays can be > 500000 elements (and
> + * causes merge to throw when larger than half a million elements),

Re "causes merge to throw": the whole point of this merge function is to avoid *push* throwing, so this merge function had better *not* throw.

I think you can reword this sentence explaining why we do things this way as "Using Array.prototype.push.apply fails when the arguments array is very large, so instead push one at a time and trust in the JIT to make it equivalently speedy. See bug 1166823."

@@ +394,5 @@
> + *
> + * @param {Array} array1
> + * @param {Array} array2
> + */
> +function merge (array1, array2) {

Can we rename this `pushAll(dest, src)`? That's a lot more clear to me, as a reader.
Attachment #8608284 - Flags: review?(nfitzgerald) → review+
Comments addressed
Attachment #8608284 - Attachment is obsolete: true
Attachment #8608328 - Flags: review+
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/bf9deffdf40b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Flags: qe-verify-
Comment on attachment 8608328 [details] [diff] [review]
1166823-array-concat.patch


Approval Request Comment
[Feature/regressing bug #]: 1167252, the new performance tool
[User impact if declined]: Won't ship the performance tool
[Describe test coverage new/current, TreeHerder]: There are try pushes in Bug 1167252 with all patches needing uplift
[Risks and why]: Requesting uplift for the accumulated changes in the performance tool since the 40 merge date, so these changes haven't had the full 6 weeks to bake.  Risks are generally contained within devtools, specifically within the performance panel.
[String/UUID change made/needed]: None
Attachment #8608328 - Flags: approval-mozilla-aurora?
Note: I had verbal confirmation for these uplifts from Sylvestre even before he's flagged them as a+.  See https://bugzilla.mozilla.org/show_bug.cgi?id=1167252#c26
Comment on attachment 8608328 [details] [diff] [review]
1166823-array-concat.patch

Change approved to skip one train as part of the spring campaign.
Attachment #8608328 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.