Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2785043002:
Increase the stack size to 2MB for x64 Windows (Closed)
DescriptionIncrease the stack size to 2MB for x64 Windows
This patch increases the stack size to 2MB for x64 Windows to reduce
stack overflow crashes.
jgruber@ commented in crbug.com/704536#c25 that V8 build has this
change. This patch uses the same value as V8.
On other platforms, as far as GetDefaultThreadStackSize() tells:
* macOS normally uses 8MB.
* Linux normally uses 8 or 16MB.
* Android uses 2MB.
Assuming comments there are correct, this change should make Windows
closer to other platforms.
Tested by "dumpbin /headers" to locally built chrome.exe:
100000 size of stack reserve
changes to:
200000 size of stack reserve
Both numbers are in hexadecimal.
BUG=704536
Review-Url: https://codereview.chromium.org/2785043002
Cr-Commit-Position: refs/heads/master@{#460828}
Committed: https://chromium.googlesource.com/chromium/src/+/bc85b8b89d33b7a7b8da86268fc30ba4fac70c87
Patch Set 1 #
Messages
Total messages: 16 (11 generated)
The CQ bit was checked by kojii@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== WIP: stacksize BUG= ========== to ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. This change should make Windows closer to other platforms. BUG=704536 ==========
Description was changed from ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. This change should make Windows closer to other platforms. BUG=704536 ========== to ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. Assuming comments there are correct, this change should make Windows closer to other platforms. BUG=704536 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. Assuming comments there are correct, this change should make Windows closer to other platforms. BUG=704536 ========== to ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. Assuming comments there are correct, this change should make Windows closer to other platforms. Tested by "dumpbin /headers" to locally built chrome.exe: 100000 size of stack reserve changes to: 200000 size of stack reserve Both numbers are in hexadecimal. BUG=704536 ==========
kojii@chromium.org changed reviewers: + brucedawson@chromium.org, dominicc@chromium.org
PTAL. I did my investigation as much as I can, but not really familiar with this kind of changes. Appreciate feedback.
I tested this by doing a local x64 build of Chrome, before and after. I then used the sysinternals vmmap tool to display all of the stacks for various Chrome processes (GPU, renderer, and browser). This confirmed that all of the stacks that used to be 1 MB are now 2 MB. The GPU process has a number of threads with stacks that are just 64 KB. Those are unaffected. I didn't investigate who was creating those and they don't seem relevant. I didn't test an x86 build of Chrome but the check seems straightforward and I wouldn't expect any difference there. In most cases this should increase address-space consumption but should not increase memory consumption so, for x64 builds, this is a good and safe change, especially since attention is being paid to minimizing stack usage. The only other thing that could be considered would be raising the stack size just for the threads that need it, but I don't think that is necessary. So, lgtm.
Thank you Bruce!!
The CQ bit was checked by kojii@chromium.org
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": 1, "attempt_start_ts": 1490897632219130, "parent_rev":
"0046d99e3911d416d0b5cb6f6f0191317bf41711", "commit_rev":
"bc85b8b89d33b7a7b8da86268fc30ba4fac70c87"}
Message was sent while issue was closed.
Description was changed from ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. Assuming comments there are correct, this change should make Windows closer to other platforms. Tested by "dumpbin /headers" to locally built chrome.exe: 100000 size of stack reserve changes to: 200000 size of stack reserve Both numbers are in hexadecimal. BUG=704536 ========== to ========== Increase the stack size to 2MB for x64 Windows This patch increases the stack size to 2MB for x64 Windows to reduce stack overflow crashes. jgruber@ commented in crbug.com/704536#c25 that V8 build has this change. This patch uses the same value as V8. On other platforms, as far as GetDefaultThreadStackSize() tells: * macOS normally uses 8MB. * Linux normally uses 8 or 16MB. * Android uses 2MB. Assuming comments there are correct, this change should make Windows closer to other platforms. Tested by "dumpbin /headers" to locally built chrome.exe: 100000 size of stack reserve changes to: 200000 size of stack reserve Both numbers are in hexadecimal. BUG=704536 Review-Url: https://codereview.chromium.org/2785043002 Cr-Commit-Position: refs/heads/master@{#460828} Committed: https://chromium.googlesource.com/chromium/src/+/bc85b8b89d33b7a7b8da86268fc3... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/bc85b8b89d33b7a7b8da86268fc3... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
