Closed
Bug 1166823
Opened 9 years ago
Closed 9 years ago
"RangeError: arguments array passed to Function.prototype.apply is too large" when recording octane
Categories
(DevTools :: Performance Tools (Profiler/Timeline), defect)
DevTools
Performance Tools (Profiler/Timeline)
Tracking
(firefox40 fixed, firefox41 fixed)
RESOLVED
FIXED
Firefox 41
People
(Reporter: fitzgen, Assigned: jsantell)
References
Details
Attachments
(1 file, 1 obsolete file)
3.41 KB,
patch
|
jsantell
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•9 years ago
|
||
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.
Reporter | ||
Comment 2•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8608284 -
Flags: review?(nfitzgerald)
Reporter | ||
Comment 4•9 years ago
|
||
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+
Assignee | ||
Comment 5•9 years ago
|
||
Comments addressed
Attachment #8608284 -
Attachment is obsolete: true
Attachment #8608328 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Whiteboard: [fixed-in-fx-team]
Comment 7•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bf9deffdf40b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 41
Updated•9 years ago
|
Blocks: perf-40-uplifts
Reporter | ||
Updated•9 years ago
|
Flags: qe-verify-
Comment 8•9 years ago
|
||
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?
Comment 9•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/4d7df5d653a6
status-firefox40:
--- → fixed
Comment 10•9 years ago
|
||
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 11•9 years ago
|
||
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+
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•