Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CesiumJS should not use push.apply for potentially large arrays #12053

Open
javagl opened this issue Jun 26, 2024 · 5 comments · May be fixed by #12361
Open

CesiumJS should not use push.apply for potentially large arrays #12053

javagl opened this issue Jun 26, 2024 · 5 comments · May be fixed by #12361
Labels
cleanup good first issue An opportunity for first time contributors JTC type - bug

Comments

@javagl
Copy link
Contributor

javagl commented Jun 26, 2024

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.

@ggetz ggetz added the cleanup label Jul 1, 2024
@javagl
Copy link
Contributor Author

javagl commented Jul 2, 2024

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...)

@ggetz ggetz added good first issue An opportunity for first time contributors JTC labels Aug 19, 2024
@Tim-Quattrochi
Copy link

Can I take on this issue, please?

@ggetz
Copy link
Contributor

ggetz commented Aug 26, 2024

@Tim-Quattrochi Yes, go ahead!

@Tim-Quattrochi
Copy link

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

@javagl
Copy link
Contributor Author

javagl commented Aug 30, 2024

@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...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup good first issue An opportunity for first time contributors JTC type - bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants