CesiumJS should not use `push.apply` for potentially large arrays

[javagl]

The issue actually is about a RangeError: Maximum call stack size exceeded. But I'll go out on a limb and write this directly as the proposed solution here.

There are several places where the push.apply is used for potentially large arrays. But push.apply puts the arguments on the stack! (The same as with the ...spread operator). That's not good.

The following is an example to reproduce the main problem:

function test() {
  const targetArray = [];
  const sourceArray = [];
  for (let i=0; i<150000; i++) {
    sourceArray.push(i);
  }
  // eslint-disable-next-line prefer-spread
  targetArray.push.apply(targetArray, sourceArray);

}

try {
  test();
} catch (error) {
  console.log(error);
}

Just running it with node _CallStackTest.js will cause the RangeError: Maximum call stack size exceeded

The push.apply pattern should be replaced by a for-loop. Preferably, with a target array with a pre-allocated size (offering an additional potential performance benefit - to be benchmarked if necessary...).

(An aside: As seen in the code snippet, there's actually an eslint rule for that - but the ...spread operator would not solve the issue here!)

---

One specific example of this pattern is in GltfLoader.parse:

// Gather promises and handle any errors
const readyPromises = [];
readyPromises.push.apply(readyPromises, loader._loaderPromises);

This actually did trigger the RangeError for a real-world glTF model that contained >150000 accessors. While such a model may certainly fall into the category of Models that challenge engine's performance or runtime limits , it should not cause a crash.

Simply replacing the above snippet with

  // Gather promises and handle any errors
  const readyPromises = [];
  const n = loader._loaderPromises.length;
  for (let i=0; i<n; i++) {
    readyPromises.push(loader._loaderPromises[i]);
  }

solved the error. But of course, there are other places that may have to be fixed.

---

If someone wants to try it out: Here is an artificial model that causes the error:

callStackTest_250x250.zip

---

Note: This was sparked by a forum thread at https://community.cesium.com/t/maximum-call-stack-size-exceeded-rangeerror/33315 , but should be considered more holistically.

[javagl]

Also reported in https://community.cesium.com/t/maximum-call-stack-size-exceeded-rangeerror/33315/13 , but I have not yet checked that model and where exactly the error comes from (but I'd place my bet on another instance of push.apply here...)

[Tim-Quattrochi]

Can I take on this issue, please?

[ggetz]

@Tim-Quattrochi Yes, go ahead!

[Tim-Quattrochi]

Just providing a WIP update, I am still working on this.

[javagl]

@Tim-Quattrochi You can (if you want to, and only if you want to) create a 'Draft' pull request for your work in progress. This way, people can track the progress and have a look at the intermediate state. (For larger, "structural" changes, it can be a good way to gather early feedback, but it might not be sooo important here: The changes are probably 'uncontroversial', insofar that it's pretty clear what the changes should look like...)