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

Status: RESOLVED FIXED

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

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

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.

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 3

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1166823-array-concat.patch

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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.

Comment 5

[Jordan Santell [:jsantell] [@jsantell]]

Attachment: 1166823-array-concat.patch

Comments addressed

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/bf9deffdf40b

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bf9deffdf40b

Comment 8

[Brian Grinstead [:bgrins]]

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

Comment 9

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4d7df5d653a6

Comment 10

[Brian Grinstead [:bgrins]]

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

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8608328 [details] [diff] [review]
1166823-array-concat.patch

Change approved to skip one train as part of the spring campaign.