镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

"inconvenient" stack overflow when using spread operator - changing stack size does not work #16870

Description

@olydis
  • Version: 7.10 (or most likely earlier) until 9.1.0 as far as I can tell
  • Platform: Windows 10
  • Subsystem: x64

Happy about recent ES features, we did stuff like someArray.push(...someOtherArray) which turns out to be problematic for large someOtherArray (say, 1 million elements), causing a stack overflow presumably because of the huge number of "arguments". There is the obvious workaround of loop-pushing (...with some helper method, since it's ugly to do that in tons of places), but given that the spread array doesn't even have too large for this to happen, I was also investigating stack size. I read about the --stack-size/--stack_size parameter, but setting that to anything significantly larger than 1000 on my Windows box caused the process to just silently crash instead of getting the nice stack overflow error.

I'm not expecting any magic, the error and its reason makes sense to me, I just wondered:

  • is there a plan regarding enabling scenarios like push(...LARGE_ARRAY) since I assume that's the pattern people will happily use these days, unaware of the caveat
  • is there any plan/way to support larger stacks on Windows?

Now that I wrote this, I think maybe these questions make more sense on the V8 side, but maybe you have valuable insights/ideas to share 🙂

Activity

  1. added
    questionIssues asking questions about Node.js.
    v8 engineIssues and PRs related to the V8 dependency.
    on Nov 7, 2017
  2. addaleax commented on Nov 7, 2017

    @addaleax
    Member

    I read about the --stack-size/--stack_size parameter, but setting that to anything significantly larger than 1000 on my Windows box caused the process to just silently crash instead of getting the nice stack overflow error.

    --stack-size doesn’t let you set the stack size; it tells V8 what to assume for the usable stack size. So when you set it to a value that exceed the actual size, V8 just runs past the end of the stack and crashes.

    /cc @nodejs/v8 for the question about .push(...array)

  3. olydis commented on Nov 7, 2017

    @olydis
    Author

    Thanks for the clarification about stack size - it felt that way given that it crashed silently 👍

  4. hashseed commented on Nov 8, 2017

    @hashseed
    Member

    This is working as intended. The spread operator simply pushes all array elements onto the stack. You essentially have the same restrictions as Function.prototype.apply. You are better served doing something like

    for (let e of someOtherArray) someArray.push(e)
    
  5. bmeurer commented on Nov 8, 2017

    @bmeurer
    Member

    How about using array spread instead?

    someArray = [...someArray, ...someOtherArray];
  6. psmarshall commented on Nov 8, 2017

    @psmarshall
    Contributor

    Yeah this is one of the ways in which spread calls are surprising in JS. There is some more discussion about it on this bug.

    • Is there a plan for allowing larger spreads?

    Basically, no. In the bug above I mention a few things we thought about to enable that, but the benefits are kind of unclear and it won't be particularly easy. Other JS engines would also need to do the same thing to enable spread to be used widely in this way. I guess for node you know much more about the JS engine you are using, so it would be more useful there.

    Unfortunately, the best advice I can give is to avoid spread calls unless you know something about the length of your array and thus the stack space it will use. It is still useful for some other things particularly in combination with rest parameters, but this style where you call Array push or Math max/min is a bit dangerous for exactly the reason you describe.

  7. olydis commented on Nov 8, 2017

    @olydis
    Author

    @hashseed @bmeurer Absolutely, note that this was not about how to work around this issue, but whether there are plans to support large spreads in calls. We used them "accidentally" since they are super convenient and while it makes perfect sense that you're exploding the stack, it's easy to miss. Also, tons of online resources will suggest stuff like that as the "ES6-way" of doing things, so I was curious whether there are plans to, say, implement calls differently in the future to work around that.

    @psmarshall Thanks for the insights, that makes perfect sense!

  8. added a commit that references this issue on Oct 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    questionIssues asking questions about Node.js.v8 engineIssues and PRs related to the V8 dependency.

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions