worker: set stack size for worker threads #26049

addaleax · 2019-02-11T19:05:09Z

~~(The first commit is #26037.)~~

This is so we can inform V8 about a known limit for the stack.

Otherwise, on some systems recursive functions may lead to
segmentation faults rather than “safe” failures.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

nodejs-github-bot · 2019-02-11T19:05:12Z

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2572/pipeline

addaleax · 2019-02-11T19:20:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20733/

test/parallel/test-worker-stack-overflow.js

src/node_worker.cc

gireeshpunathil · 2019-02-12T06:57:09Z

src/node_worker.h

+  uintptr_t stack_base_;
+
+  // Full size of the thread's stack.
+  static constexpr size_t kStackSize = 4 * 1024 * 1024;


For platforms where 4MB is not the default stack size, does this prove as an added memory pressure, and if so, should we document this?

does this prove as an added memory pressure

@gireeshpunathil Not on most platforms in their default configuration, I assume – usually, memory pages are only actually committed when first accessed.

I wouldn’t explicitly document this, as it shouldn’t make much of a difference for users.

gireeshpunathil · 2019-02-12T06:59:09Z

src/node_worker.cc

    Worker* w = static_cast<Worker*>(arg);
+    const uintptr_t stack_top = reinterpret_cast<uintptr_t>(&arg);


Is it possible that some compilers decide to take this address from the caller thread's stack as opposed to the current thread's stack? I guess it should not, but I am not up-to-date with compiler optimizations.

The compiler doesn’t know whether there are different threads involved here. And if there’s something invalid about this, then V8 already has the same problem in their code (which I assume they don’t).

gireeshpunathil · 2019-02-12T07:04:35Z

Should / Could we also expose this into the JS land? say: new Worker(... {stack: <size>});
that way, different workers could be spawned that vary in thread stack size based on the workload that they specializes on? Users can calibrate the values for specific tasks, and thereby optimize memory (in deployments where memory costs).

test/parallel/test-worker-stack-overflow.js

bnoordhuis · 2019-02-12T09:22:51Z

Should / Could we also expose this into the JS land? say: new Worker(... {stack: <size>});

Too small or too big stack sizes can be dangerous and the limits depend on the platform and other factors.

There's also the question of how a programmer can know what a reasonable stack size is for a given workload without knowing all about Node's inner workings, or the add-ons they use.

addaleax · 2019-02-13T12:30:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/20760/

This is so we can inform V8 about a known limit for the stack. Otherwise, on some systems recursive functions may lead to segmentation faults rather than “safe” failures.

addaleax · 2019-02-13T13:39:36Z

Rebased, new CI: https://ci.nodejs.org/job/node-test-pull-request/20763/

addaleax · 2019-02-13T20:40:18Z

Landed in a80b29b

(If this should get backported to v10.x at some point, it depends on #26037.)

This is so we can inform V8 about a known limit for the stack. Otherwise, on some systems recursive functions may lead to segmentation faults rather than “safe” failures. PR-URL: #26049 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

ulrichb · 2020-01-30T13:41:12Z

So, there is now a hard-coded stack size limit of 4 MB, right?

Is there any possibility to override this limit (e.g. using worker.resourceLimits)?

addaleax · 2020-01-30T16:00:53Z

@ulrichb There isn’t, currently – as you can see, there was a brief discussion above about that: #26049 (comment)

If you’d like to see a feature like that, could you open a new issue and ideally provide some information about your use case?

ulrichb · 2020-01-31T09:56:41Z

@addaleax I'm not quite sure why, but it seems that the stack limit can be influenced by the V8 switch --stack-size also for the worker threads. Could you confirm that? (Using Node 13.7)

BTW: My use case is that I'm hosting the TypeScript compiler in a worker thread which does a lot of recursion, and this hit the stack limit for really big files (I have generated code files with ~ 20kLOC.)

ulrichb · 2020-01-31T10:03:42Z

... oh and I used --stack-size=2048 (2 MB), to solve my RangeError: Maximum call stack size exceeded exceptions. This is strange because it's more than the default for --stack-size, but it's less than the 4 MB of this pull request. (And yes, I'm sure that the error happens within the worker thread.)

@addaleax Any ideas? Do the two values (--stack-size and kStackSize) mean something slightly different? Or corresponds --stack-size to kStackBufferSize (192 kB)?

addaleax · 2020-01-31T18:17:30Z

@ulrichb So…

--stack-size does not set the actual stack size; instead, it tells V8 about what the stack size is, mostly so that it knows when to bail out with a stack overflow exception.
kStackSize is the full stack size that is natively allocated for a Worker thread.
kStackBufferSize refers to a part of the full stack that is not free for use by V8, and instead only there so that C++ code that does not perform stack checks can still reasonably run when near the stack limit.

--stack-size corresponds to kStackSize - kStackBufferSize, then.

This is strange because it's more than the default for --stack-size, but it's less than the 4 MB of this pull request. (And yes, I'm sure that the error happens within the worker thread.)

I’m not sure what to say except that I fully agree with you – this is strange. I feel like --stack-size shouldn’t even have an effect on Worker threads at all, although it defintiely does. I think that qualifies as a separate bug, I’ll look into it.

One thing that I can tell you from anecdotal experience is that I’ve also been in situations in which lowering --stack-size actually removed stack overflow exceptions surfacing; the reason being that they then were thrown from another part of the program that was part of a try/catch. Something like that may or may not be the case here.

It turns out that using `v8::Locker` undoes the effects of passing an explicit stack limit as part of the `Isolate`’s resource constraints. Therefore, reset the stack limit manually after entering a Locker. Refs: nodejs#26049 (comment)

It turns out that using `v8::Locker` undoes the effects of passing an explicit stack limit as part of the `Isolate`’s resource constraints. Therefore, reset the stack limit manually after entering a Locker. Refs: #26049 (comment) PR-URL: #31593 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]>

addaleax added blocked PRs that are blocked by other issues or PRs. worker Issues and PRs related to Worker support. labels Feb 11, 2019

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 11, 2019

addaleax removed the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 11, 2019

jasnell approved these changes Feb 11, 2019

View reviewed changes

cjihrig reviewed Feb 11, 2019

View reviewed changes

test/parallel/test-worker-stack-overflow.js Outdated Show resolved Hide resolved

src/node_worker.cc Outdated Show resolved Hide resolved

gireeshpunathil reviewed Feb 12, 2019

View reviewed changes

bnoordhuis approved these changes Feb 12, 2019

View reviewed changes

test/parallel/test-worker-stack-overflow.js Outdated Show resolved Hide resolved

cjihrig approved these changes Feb 12, 2019

View reviewed changes

worker: set stack size for worker threads

23c4573

This is so we can inform V8 about a known limit for the stack. Otherwise, on some systems recursive functions may lead to segmentation faults rather than “safe” failures.

addaleax force-pushed the worker-stack-overflow branch from b1c60cb to 23c4573 Compare February 13, 2019 13:38

addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed blocked PRs that are blocked by other issues or PRs. labels Feb 13, 2019

addaleax closed this Feb 13, 2019

addaleax deleted the worker-stack-overflow branch February 13, 2019 20:40

targos mentioned this pull request Feb 14, 2019

v11.10.0 proposal #26098

Merged

beevelop mentioned this pull request Feb 15, 2019

2019-02-14 Version 11.10.0 (Current) @targos beevelop/docker-android-nodejs#290

Closed

addaleax mentioned this pull request Jan 31, 2020

worker: reset Isolate stack limit after entering Locker #31593

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

worker: set stack size for worker threads #26049

worker: set stack size for worker threads #26049

addaleax commented Feb 11, 2019 •

edited

Loading

nodejs-github-bot commented Feb 11, 2019

addaleax commented Feb 11, 2019

gireeshpunathil Feb 12, 2019

addaleax Feb 12, 2019

gireeshpunathil Feb 12, 2019

addaleax Feb 12, 2019

gireeshpunathil commented Feb 12, 2019

bnoordhuis commented Feb 12, 2019

addaleax commented Feb 13, 2019

addaleax commented Feb 13, 2019

addaleax commented Feb 13, 2019

ulrichb commented Jan 30, 2020

addaleax commented Jan 30, 2020

ulrichb commented Jan 31, 2020

ulrichb commented Jan 31, 2020 •

edited

Loading

addaleax commented Jan 31, 2020

		Worker* w = static_cast<Worker*>(arg);
		const uintptr_t stack_top = reinterpret_cast<uintptr_t>(&arg);

worker: set stack size for worker threads #26049

worker: set stack size for worker threads #26049

Conversation

addaleax commented Feb 11, 2019 • edited Loading

Checklist

nodejs-github-bot commented Feb 11, 2019

addaleax commented Feb 11, 2019

gireeshpunathil Feb 12, 2019

Choose a reason for hiding this comment

addaleax Feb 12, 2019

Choose a reason for hiding this comment

gireeshpunathil Feb 12, 2019

Choose a reason for hiding this comment

addaleax Feb 12, 2019

Choose a reason for hiding this comment

gireeshpunathil commented Feb 12, 2019

bnoordhuis commented Feb 12, 2019

addaleax commented Feb 13, 2019

addaleax commented Feb 13, 2019

addaleax commented Feb 13, 2019

ulrichb commented Jan 30, 2020

addaleax commented Jan 30, 2020

ulrichb commented Jan 31, 2020

ulrichb commented Jan 31, 2020 • edited Loading

addaleax commented Jan 31, 2020

addaleax commented Feb 11, 2019 •

edited

Loading

ulrichb commented Jan 31, 2020 •

edited

Loading