Replace push.apply with dedicated function

[javagl]

Fixes #12053
Makes #12176 obsolete

Description

The title of the issue is "CesiumJS should not use push.apply for potentially large arrays" , which bears the question of what is a "large" array. Given that the answer to that question is the same as to "How long is a piece of string?", this PR just replaces all appearances of
target.push.apply(target, source);
with
addAll(source, target);
using a newly introduced addAll function.

This addAll function is a top-level function, but marked as @private for now.

Issue number and link

#12053

Testing plan

The case where this issue originally came up was that of a glTF with many accessors: Each accessor causes a 'promise' to be created, and these promises had been pushed to a target array with push.apply. For many accessors, this caused a RangeError, as described in the issue.

The following is a sandcastle that loads such a glTF asset, and can be used to verify that the issue no longer appears:

const viewer = new Cesium.Viewer("cesiumContainer", {
  globe: false
});

const url = "http://localhost:8003/callStackTest_250x250.glb";

const camera = viewer.scene.camera;
camera.position = new Cesium.Cartesian3(500, 140, 140);
camera.direction = Cesium.Cartesian3.negate(
  Cesium.Cartesian3.UNIT_X, new Cesium.Cartesian3());
camera.up = Cesium.Cartesian3.clone(Cesium.Cartesian3.UNIT_Z);
camera.frustum.fov = Cesium.Math.PI_OVER_THREE;
camera.frustum.near = 0.01;
camera.frustum.far = 1000.0;
camera.lookAtTransform(Cesium.Matrix4.IDENTITY);

let totalDurationMs = 0;


async function addModel(runs, currentRun) {
  if (currentRun === runs) {
    const averageDurationMs = totalDurationMs / runs;
    console.log("runs: "+runs);
    console.log("totalDurationMs: "+totalDurationMs);
    console.log("averageDurationMs: "+averageDurationMs);
  } else {
    
    console.log("Adding, run " + currentRun + " of " + runs);
    const before = performance.now();
    const model = await Cesium.Model.fromGltfAsync({
      url: url,
    });
    viewer.scene.primitives.add(model);

    model.readyEvent.addEventListener(async () => {
      const after = performance.now();
      const durationMs = after-before;
      console.log("ready, took "+durationMs+" ms");
      totalDurationMs += durationMs;
      if (currentRun < runs - 1) {
        viewer.scene.primitives.remove(model);
      }
      await addModel(runs, currentRun + 1);
    });
  }  
}

addModel(1, 0);
//addModel(10, 0);

The following archive contains the main asset for the test:

callStackTest.zip

When using the callStackTest_250x250.glb, then the current main state causes an error. With the fixed state, it works.

---

The archive contains a few smaller assets as well, and the sandcastle offers a very basic "benchmarking" functionality, to repeatedly load an asset. This is intended for checking whether this change has a noticable performance impact on this particular code path. This makes limited sense: There have been many places that used push.apply. Many of them never saw anything that even remotely was a "large" array. In the specific case of the glTF models, most of the time was not spent in push.apply, but with loading the data to begin with. But as a baseline for this case that can easily be tested (beyond some "microbenchmarks", as shown in the linked PR), I ran this test with the 120x120 asset, and there doesn't seem to be a significant performance impact.

---

Author checklist

• I have submitted a Contributor License Agreement
• I have added my name to CONTRIBUTORS.md
• I have updated CHANGES.md with a short summary of my change
• I have added or updated unit tests to ensure consistent code coverage
• I have updated the inline documentation, and included code examples where relevant
• I have performed a self-review of my code

[github-actions]

Thank you for the pull request, @javagl!

✅ We can confirm we have a CLA on file for you.

[ggetz]

Thanks @javagl!

@jjspace Can you please review?

[jjspace]

I tested with the asset you provided and did confirm it works on this branch and not in main. I did have a few comments about the new function itself though.

[ggetz]

@javagl Is this ready for review and potential merge again?

[javagl]

@ggetz Fine to merge from my perspective, if the comments by @jjspace are resolved.

A tangential connection to #8359 : One of the rules/transforms of Lebap is

arg-spread - use of apply() to spread operator

Which changes this:

But... as it turned out: The ...spread operator also places the arguments on the stack - so this wouldn't solve the issue that triggered this. It could be a rewrite for readability and more idiomatic code.

An aside: I considered to review the code for cases where normal for-loops could now be replaced by addAllToArray. When they didn't use push.apply, they aren't caught in this PR, and could still benefit in terms of readabiltiy and maybe also performance, due to the pre-allocation of the required .length. But I'm not sure whether searching and identifying these places would be worth the effort...

[ggetz]

Fine to merge from my perspective, if the comments by @jjspace are resolved.

Great, thank you! @jjspace this is pending another pass from you,

[ggetz]

A tangential connection to #8359 : One of the rules/transforms of Lebap is...

I think this is a good reason to be targeted about modernization pushes we do, and to not blanket apply transforms: We need to ensure that there are no performance implications when we do so.

When they didn't use push.apply, they aren't caught in this PR, and could still benefit in terms of readabiltiy and maybe also performance, due to the pre-allocation of the required .length

I agree at minimum, this is out of scope with the original scope of this PR and there's no need to do it here. Unless we confirm that there is a performance implication to motivate any other use case, I think we're getting into bikeshedding territory and we should hold off.

[jjspace]

This is looking good now. Just 2 comments:

1. Should there be a section added to our Coding Guide (or some other guide) explaining why we want to avoid .push.apply? maybe linking back to the discussion in this PR or the issue?
2. Optional change but should we actually restrict usage of this function through eslint? It's possible to do that through the no-restricted-syntax rule with either a warning or a full error. @ggetz thoughts on this?

rules: {
  "no-restricted-syntax": [
    "warn",
    {
      // Link to issue here
      selector:
        "CallExpression[callee.object.property.name=push][callee.property.name=apply]",
      message:
        "We prefer to avoid Array.push.apply() in favor of addAllToArray()",
    },
  ],
},

[javagl]

@jjspace No strong opinion from my side, just minor thoughts:

As of my comment above, one 'Lebap' modernization transform allows converting push.apply to ...spread automatically. So one could make a case that the Coding Guide could just have a hint like "Write modern, idiomatic code". But... the ...spread operator could similarly be discouraged. So these points may indeed be worth mentioning. (I'm pretty sure that even many experienced developers don't have that 'call stack' issue on the radar...).

(BTW: I wasn't aware of the no-restricted-syntax and that it could be defined so easily (well... "easy", when you know the syntax for it 😁 ))

Maybe we should just collect some ideas in some "Possible updates for the Coding Guide" issue, to better keep track of that? (I mean, this could also include other "modernization" hints, like "Don't do array.indexOf(element) !== -1, but array.includes(element)" or so....)

[jjspace]

(I'm pretty sure that even many experienced developers don't have that 'call stack' issue on the radar...).

@javagl I think this is exactly why it would be worth calling out in the coding guide somewhere. I don't think we need to add a section on modernization in general. It would be more on this as a performance topic like the original issue.

Maybe we should just collect some ideas in some "Possible updates for the Coding Guide" issue, to better keep track of that?

If you have other immediate ideas of things to add then it could be worth opening an issue but I think it's more likely to actually get in if we add it to a PR directly. I think something should be added alongside this change just focused on the pitfall of Array.push.apply. I'm not entirely sure what heading it should go under so maybe a new one makes sense even if this is the only topic in it for now. Maybe just a general "Performance" section

I also do think we should add the eslint rule but only as a warning. This will ensure it's automated and shows up in the actual code instead of relying on everyone remembering everything in the coding guide at all times. The codeblock above will achieve this, I think it can be added to the files: ["packages/**/*.js"... section. Just please make sure to leave comments there explaining why and point to the original issue.

Once those changes are in I think this should be good to merge

[javagl]

@jjspace I added that linting rule as you suggested. I tried to elaborate the rule/warning text a bit:

Avoid Array.push.apply(). Use addAllToArray() for arrays of unknown size, or the spread syntax for arrays that are known to be small

The spread syntax can be nice, modern, and convenient, and for something like
nodes.push([...binaryTree.children]); // Binary = At most 2 elements
they can be fine, so I tried to give a "reasonable" guidance in that message.

For some potential further updates of the Coding Guide (including push.apply hints), I opened #12452

[jjspace]

LGTM, sorry this took so long to get back to. Also surprised there were no merge conflicts but it looks good 😄