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.

[jjspace]

I don't think I like the name addAll. I don't feel it's descriptive enough to understand what it does without looking at the documentation or the function itself. addAllToArray or maybe combineArrays could be better? Or, given my previous comment on this topic with this essentially doing what Array.concat does, maybe concatInto or concatIntoArray would be more accurate?

[javagl]

No really strong opinions, just things that I thought about:

• In the linked (soon to be obsolete) PR, I initially called that function pushAll, but I wanted to emphasize that it does exactly not push...
• Any name that involves concat could easily be confused with Array.concat, and (as discussed in the other PR), that has a completely different semantic: People might expect it to return a concatenation. I think it's important to make clear that it operates in-place, and on the given target array
• combine... could be OK (maybe people would expect it to return something there, but not necessarily)

Maybe something like appendAll or append(All?To?)Array or so ...?

[javagl]

Changed to addAllToArray - sounds reasonable and is long and clear enough for a global function.

[jjspace]

it's a nitpick but in general I think we should avoid single letter variable names outside the looping variables i, j, k, and sometimes l.

Maybe it's just personal preference but I find this a little more readable even if it is more verbose

  const sourceLength = source.length;
  if (sourceLength === 0) {
    return;
  }
  const targetLength = target.length;
  target.length += sourceLength;
  for (let i = 0; i < sourceLength; i++) {
    target[targetLength + i] = source[i];
  }

[javagl]

I nearly espected that, and can change it accordingly.
But regarding the variable name 1: Here is an animated GIF of l and 1, switching every second:

[javagl]

Renamed to sourceLength and targetLength

[jjspace]

Nitpick on semantics, but this time I do think it actually matters. When I first glanced at this code I expected the order of the arguments to be reversed: addAll(target, source).
This is largely because I am comparing this to similar functions of target.push(...source) or target.concat(source) etc. In all the builtin array functions the "result" is on the left and the "source" is on the right.
Another way I think about it is that this is replacing the syntax [...target, ...source] to put all the target elements first and the source elements second. So the target variable should be the first argument. Even in this example you're doing addAll([ 3, 4, 5 ], [ 0, 1, 2 ]) = [ 0, 1, 2, 3, 4, 5 ]
(Maybe "source" and "target" are the wrong names in the first place)

I think swapping them also aligns better with the idea that the "source" could be null, undefined or []. Then the first argument is always defined even if the second only sometimes is.

[javagl]

I also hesitated here. Leaving the source as the last parameter would allow omitting it, as in
addAll(target);
which is probably not ideal.

But in general, I'm open to changing it, and embarassingly have to point out that commit c00243a ...

[javagl]

From my all-time favorite presentation on API design:

[javagl]

Swapped the parameters. Fortunately, someone posted the Regex for that, which is to ...
replace (addAllToArray)\(([^,]+), *([^,]+)\)
with $1($3, $2)