Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640803002:
[base] Increase worker_pool thread stacksize to 1MB on MacOSX. (Closed)
Description[base] Increase worker_pool thread stacksize to 1MB on MacOSX.
The default stack size of a background thread is 512KB on MacOSX. We might
need at least 1MB stack size to compile JS code in V8, and since we
want to enable compilation of JS code onto background threads, we
need to up the size of the stack for worker pool threads.
BUG=v8:5203
Review-Url: https://codereview.chromium.org/2640803002
Cr-Commit-Position: refs/heads/master@{#445739}
Committed: https://chromium.googlesource.com/chromium/src/+/057e94297ee02e02b8ef4ff2dbe4c6f5f0914bab
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address comments #Messages
Total messages: 17 (8 generated)
rmcilroy@chromium.org changed reviewers: + mark@chromium.org
Mark, could you please take a look? Jochen, FYI.
Description was changed from ========== [base] Increase worker_pool thread stacksize to 1MB on MacOSX. The default stack size of a background thread is 512KB on MacOSX. We need at least 1MB stack size to compile JS code in V8, and since we want to enable compilation of JS code onto background threads, we need to up the size of the stack for worker pool threads. BUG=v8:5203 ========== to ========== [base] Increase worker_pool thread stacksize to 1MB on MacOSX. The default stack size of a background thread is 512KB on MacOSX. We might need at least 1MB stack size to compile JS code in V8, and since we want to enable compilation of JS code onto background threads, we need to up the size of the stack for worker pool threads. BUG=v8:5203 ==========
Just to give some context, on V8 we have an explicit stack check which we do during the recursive AST stack walk during compilation to ensure we can cleanly abort compilation (and throw a JavaScript exception) if we get close to the real C stack limit. This stack check is set to just under 1MB, which is fine on the main thread, however on Mac with 512KB background threads we hit the hard C stack limit before hitting this safe explicit stack check, and so can crash on compiling JS code which has a deep expression stack.
Friendly ping, could you take a look please Mark?
jochen@chromium.org changed reviewers: + jochen@chromium.org
lgtm from my side fwiw
On 2017/01/20 09:32:10, jochen wrote: > lgtm from my side fwiw Mark, could you review for base/ OWNERS please?
LGTM otherwise https://codereview.chromium.org/2640803002/diff/1/base/threading/worker_pool_... File base/threading/worker_pool_posix.cc (right): https://codereview.chromium.org/2640803002/diff/1/base/threading/worker_pool_... base/threading/worker_pool_posix.cc:34: // On MacOSX a background thread's default stack size is 512KB. We need at least macOS, OS X, or Mac OS X. and kB :) https://codereview.chromium.org/2640803002/diff/1/base/threading/worker_pool_... base/threading/worker_pool_posix.cc:36: const int kDefaultStackSize = 1 * 1024 * 1024; Don’t call it kDefaultStackSize, because it’s not a default anymore. Just kStackSize? And for non-macOS, it’s 0, which means “default.”
Patchset #2 (id:20001) has been deleted
Thanks Mark. Landing now. https://codereview.chromium.org/2640803002/diff/1/base/threading/worker_pool_... File base/threading/worker_pool_posix.cc (right): https://codereview.chromium.org/2640803002/diff/1/base/threading/worker_pool_... base/threading/worker_pool_posix.cc:34: // On MacOSX a background thread's default stack size is 512KB. We need at least On 2017/01/23 23:02:13, Mark Mentovai wrote: > macOS, OS X, or Mac OS X. > > and kB :) Done. https://codereview.chromium.org/2640803002/diff/1/base/threading/worker_pool_... base/threading/worker_pool_posix.cc:36: const int kDefaultStackSize = 1 * 1024 * 1024; On 2017/01/23 23:02:13, Mark Mentovai wrote: > Don’t call it kDefaultStackSize, because it’s not a default anymore. Just > kStackSize? And for non-macOS, it’s 0, which means “default.” Done.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org, jochen@chromium.org Link to the patchset: https://codereview.chromium.org/2640803002/#ps40001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485269561430120,
"parent_rev": "58614795c7844605fd8ecd9e5f7d36b1335893d3", "commit_rev":
"057e94297ee02e02b8ef4ff2dbe4c6f5f0914bab"}
Message was sent while issue was closed.
Description was changed from ========== [base] Increase worker_pool thread stacksize to 1MB on MacOSX. The default stack size of a background thread is 512KB on MacOSX. We might need at least 1MB stack size to compile JS code in V8, and since we want to enable compilation of JS code onto background threads, we need to up the size of the stack for worker pool threads. BUG=v8:5203 ========== to ========== [base] Increase worker_pool thread stacksize to 1MB on MacOSX. The default stack size of a background thread is 512KB on MacOSX. We might need at least 1MB stack size to compile JS code in V8, and since we want to enable compilation of JS code onto background threads, we need to up the size of the stack for worker pool threads. BUG=v8:5203 Review-Url: https://codereview.chromium.org/2640803002 Cr-Commit-Position: refs/heads/master@{#445739} Committed: https://chromium.googlesource.com/chromium/src/+/057e94297ee02e02b8ef4ff2dbe4... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/057e94297ee02e02b8ef4ff2dbe4... |
